| Message ID | 20200903163216.6359-3-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 03/09/2020 17:32, 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> > --- > src/android/camera_device.cpp | 18 ++++++++++- > 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 | 48 +++++++++++++++++++++++++++- > src/android/jpeg/exif.h | 8 +++++ > 6 files changed, 84 insertions(+), 5 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index de6f86f..54ca9c6 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,22 @@ 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)); > + exif.generate(); Hrm ... Did this pass compile tests? Didn't we add a [[nodiscard]] to this call? Although - if it does fail to generate, I think we'd still expect to call into the encoder->encode() and I think the exif.data() would be nullptr,0, (which is what we want), as the encoder checks the size to determine if it should be added or not. > + > + 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 9d8d9bc..5def7e3 100644 > --- a/src/android/jpeg/exif.cpp > +++ b/src/android/jpeg/exif.cpp > @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > exif_entry_unref(entry); > } > > -int Exif::generate() > +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 = 1; > + switch (orientation) { > + case 90: > + value = 6; > + break; > + case 180: > + value = 3; > + break; > + case 270: > + value = 8; > + break; > + } > + > + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > +} > + > +void Exif::generate() > { > if (exifData_) { > free(exifData_); > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > index 8fb8ffd..6113ca6 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_ }; } > int [[nodiscard]] generate(); > >
On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote: > Hi Umang, > > On 03/09/2020 17:32, 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> > > --- > > src/android/camera_device.cpp | 18 ++++++++++- > > 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 | 48 +++++++++++++++++++++++++++- > > src/android/jpeg/exif.h | 8 +++++ > > 6 files changed, 84 insertions(+), 5 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index de6f86f..54ca9c6 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,22 @@ 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)); > > + exif.generate(); > > Hrm ... Did this pass compile tests? Likely not, given that generate() is defined as returning int in the header file and void in the implementation. Rebase issue, or sending the wrong version ? > Didn't we add a [[nodiscard]] to this call? > > Although - if it does fail to generate, I think we'd still expect to > call into the encoder->encode() and I think the exif.data() would be > nullptr,0, (which is what we want), as the encoder checks the size to > determine if it should be added or not. > > > + > > + 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 9d8d9bc..5def7e3 100644 > > --- a/src/android/jpeg/exif.cpp > > +++ b/src/android/jpeg/exif.cpp > > @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > > exif_entry_unref(entry); > > } > > > > -int Exif::generate() > > +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 = 1; > > + switch (orientation) { > > + case 90: > > + value = 6; > > + break; > > + case 180: > > + value = 3; > > + break; > > + case 270: > > + value = 8; > > + break; > > + } > > + > > + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > > +} > > + > > +void Exif::generate() > > { > > if (exifData_) { > > free(exifData_); > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > > index 8fb8ffd..6113ca6 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_ }; } > > int [[nodiscard]] generate(); > >
Hi Umang, Thank you for the patch. On Thu, Sep 03, 2020 at 10:02:16PM +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> > --- > src/android/camera_device.cpp | 18 ++++++++++- > 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 | 48 +++++++++++++++++++++++++++- > src/android/jpeg/exif.h | 8 +++++ > 6 files changed, 84 insertions(+), 5 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index de6f86f..54ca9c6 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,22 @@ 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)); > + exif.generate(); You need to either check the error value here (but what would we do with it, maybe just printing an error ?), or drop [[nodiscard]]. > + > + 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 9d8d9bc..5def7e3 100644 > --- a/src/android/jpeg/exif.cpp > +++ b/src/android/jpeg/exif.cpp > @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > exif_entry_unref(entry); > } > > -int Exif::generate() > +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 = 1; > + switch (orientation) { case 0: default: value = 1; break; and you can remove the initialization of the variable. > + case 90: > + value = 6; > + break; > + case 180: > + value = 3; > + break; > + case 270: > + value = 8; > + break; > + } > + > + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > +} > + > +void Exif::generate() This line seems to be a rebase issue. With these small issues fixed (and the patch series tested ;-)), Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > { > if (exifData_) { > free(exifData_); > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > index 8fb8ffd..6113ca6 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_ }; } > int [[nodiscard]] generate(); >
Hi Laurent, Kieran, On 9/4/20 4:10 AM, Laurent Pinchart wrote: > On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote: >> Hi Umang, >> >> On 03/09/2020 17:32, 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> >>> --- >>> src/android/camera_device.cpp | 18 ++++++++++- >>> 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 | 48 +++++++++++++++++++++++++++- >>> src/android/jpeg/exif.h | 8 +++++ >>> 6 files changed, 84 insertions(+), 5 deletions(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index de6f86f..54ca9c6 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,22 @@ 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)); >>> + exif.generate(); >> Hrm ... Did this pass compile tests? > Likely not, given that generate() is defined as returning int in the > header file and void in the implementation. Rebase issue, or sending the > wrong version ? Indeed, a faulty rebase on my end .. /o\ Although, it *did* pass the compile test and I can confirm compiling the patches again (as is this series). It's quite weird. GCC versions are : C compiler for the host machine: cc (gcc 9.3.0 "cc (Ubuntu 9.3.0-10ubuntu2) 9.3.0") C linker for the host machine: cc ld.bfd 2.34 C++ compiler for the host machine: c++ (gcc 9.3.0 "c++ (Ubuntu 9.3.0-10ubuntu2) 9.3.0") C++ linker for the host machine: c++ ld.bfd 2.34 > >> Didn't we add a [[nodiscard]] to this call? >> >> Although - if it does fail to generate, I think we'd still expect to >> call into the encoder->encode() and I think the exif.data() would be >> nullptr,0, (which is what we want), as the encoder checks the size to >> determine if it should be added or not. >> >>> + >>> + 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 9d8d9bc..5def7e3 100644 >>> --- a/src/android/jpeg/exif.cpp >>> +++ b/src/android/jpeg/exif.cpp >>> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str >>> exif_entry_unref(entry); >>> } >>> >>> -int Exif::generate() >>> +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 = 1; >>> + switch (orientation) { >>> + case 90: >>> + value = 6; >>> + break; >>> + case 180: >>> + value = 3; >>> + break; >>> + case 270: >>> + value = 8; >>> + break; >>> + } >>> + >>> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); >>> +} >>> + >>> +void Exif::generate() >>> { >>> if (exifData_) { >>> free(exifData_); >>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h >>> index 8fb8ffd..6113ca6 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_ }; } >>> int [[nodiscard]] generate(); >>>
Hi Laurent, On 9/4/20 4:53 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Thu, Sep 03, 2020 at 10:02:16PM +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> >> --- >> src/android/camera_device.cpp | 18 ++++++++++- >> 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 | 48 +++++++++++++++++++++++++++- >> src/android/jpeg/exif.h | 8 +++++ >> 6 files changed, 84 insertions(+), 5 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index de6f86f..54ca9c6 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,22 @@ 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)); >> + exif.generate(); > You need to either check the error value here (but what would we do with > it, maybe just printing an error ?), or drop [[nodiscard]]. Yes, it's a bit of conundrum. We surely want to keep [[nodiscard]], so I would just check the value and log an error here as well. (i.e. we will get 2 errors if EXIF turns out to be invalid, one from EXIF Log domain and other one from here, HAL). > >> + >> + 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 9d8d9bc..5def7e3 100644 >> --- a/src/android/jpeg/exif.cpp >> +++ b/src/android/jpeg/exif.cpp >> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str >> exif_entry_unref(entry); >> } >> >> -int Exif::generate() >> +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 = 1; >> + switch (orientation) { > case 0: > default: > value = 1; > break; > > and you can remove the initialization of the variable. > >> + case 90: >> + value = 6; >> + break; >> + case 180: >> + value = 3; >> + break; >> + case 270: >> + value = 8; >> + break; >> + } >> + >> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); >> +} >> + >> +void Exif::generate() > This line seems to be a rebase issue. > > With these small issues fixed (and the patch series tested ;-)), Never I have ever sent patches without atleast compiling patches. (How stupid would that be!) These unfortunately turned out to be a bad rebase, but it did pass the compile test for sure! > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> { >> if (exifData_) { >> free(exifData_); >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h >> index 8fb8ffd..6113ca6 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_ }; } >> int [[nodiscard]] generate(); >>
Hi Umang, On Fri, Sep 04, 2020 at 11:35:08AM +0530, Umang Jain wrote: > On 9/4/20 4:10 AM, Laurent Pinchart wrote: > > On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote: > >> On 03/09/2020 17:32, 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> > >>> --- > >>> src/android/camera_device.cpp | 18 ++++++++++- > >>> 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 | 48 +++++++++++++++++++++++++++- > >>> src/android/jpeg/exif.h | 8 +++++ > >>> 6 files changed, 84 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>> index de6f86f..54ca9c6 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,22 @@ 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)); > >>> + exif.generate(); > >> > >> Hrm ... Did this pass compile tests? > > > > Likely not, given that generate() is defined as returning int in the > > header file and void in the implementation. Rebase issue, or sending the > > wrong version ? > > Indeed, a faulty rebase on my end .. /o\ > > Although, it *did* pass the compile test and I can confirm > compiling the patches again (as is this series). > It's quite weird. GCC versions are : > > C compiler for the host machine: cc (gcc 9.3.0 "cc (Ubuntu > 9.3.0-10ubuntu2) 9.3.0") > C linker for the host machine: cc ld.bfd 2.34 > C++ compiler for the host machine: c++ (gcc 9.3.0 "c++ (Ubuntu > 9.3.0-10ubuntu2) 9.3.0") > C++ linker for the host machine: c++ ld.bfd 2.34 Could it be that the android option is set to false in your build ? Here's what I get with gcc 9.3.0. [24/172] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' FAILED: src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o g++-9.3.0 -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -Iinclude/libcamera -I/usr/include/libexif -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' -c ../../src/android/jpeg/exif.cpp In file included from ../../src/android/jpeg/exif.cpp:8: ../../src/android/jpeg/exif.h:32:6: error: attribute ignored [-Werror=attributes] 32 | int [[nodiscard]] generate(); | ^ ../../src/android/jpeg/exif.h:32:6: note: an attribute that appertains to a type-specifier is ignored ../../src/android/jpeg/exif.cpp:218:6: error: no declaration matches ‘void Exif::generate()’ 218 | void Exif::generate() | ^~~~ In file included from ../../src/android/jpeg/exif.cpp:8: ../../src/android/jpeg/exif.h:32:21: note: candidate is: ‘int Exif::generate()’ 32 | int [[nodiscard]] generate(); | ^~~~~~~~ ../../src/android/jpeg/exif.h:18:7: note: ‘class Exif’ defined here 18 | class Exif | ^~~~ cc1plus: all warnings being treated as errors > >> Didn't we add a [[nodiscard]] to this call? > >> > >> Although - if it does fail to generate, I think we'd still expect to > >> call into the encoder->encode() and I think the exif.data() would be > >> nullptr,0, (which is what we want), as the encoder checks the size to > >> determine if it should be added or not. > >> > >>> + > >>> + 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 9d8d9bc..5def7e3 100644 > >>> --- a/src/android/jpeg/exif.cpp > >>> +++ b/src/android/jpeg/exif.cpp > >>> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str > >>> exif_entry_unref(entry); > >>> } > >>> > >>> -int Exif::generate() > >>> +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 = 1; > >>> + switch (orientation) { > >>> + case 90: > >>> + value = 6; > >>> + break; > >>> + case 180: > >>> + value = 3; > >>> + break; > >>> + case 270: > >>> + value = 8; > >>> + break; > >>> + } > >>> + > >>> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); > >>> +} > >>> + > >>> +void Exif::generate() > >>> { > >>> if (exifData_) { > >>> free(exifData_); > >>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > >>> index 8fb8ffd..6113ca6 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_ }; } > >>> int [[nodiscard]] generate(); > >>>
Hi Laurent, On 9/4/20 6:14 PM, Laurent Pinchart wrote: > Hi Umang, > > On Fri, Sep 04, 2020 at 11:35:08AM +0530, Umang Jain wrote: >> On 9/4/20 4:10 AM, Laurent Pinchart wrote: >>> On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote: >>>> On 03/09/2020 17:32, 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> >>>>> --- >>>>> src/android/camera_device.cpp | 18 ++++++++++- >>>>> 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 | 48 +++++++++++++++++++++++++++- >>>>> src/android/jpeg/exif.h | 8 +++++ >>>>> 6 files changed, 84 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>>> index de6f86f..54ca9c6 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,22 @@ 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)); >>>>> + exif.generate(); >>>> Hrm ... Did this pass compile tests? >>> Likely not, given that generate() is defined as returning int in the >>> header file and void in the implementation. Rebase issue, or sending the >>> wrong version ? >> Indeed, a faulty rebase on my end .. /o\ >> >> Although, it *did* pass the compile test and I can confirm >> compiling the patches again (as is this series). >> It's quite weird. GCC versions are : >> >> C compiler for the host machine: cc (gcc 9.3.0 "cc (Ubuntu >> 9.3.0-10ubuntu2) 9.3.0") >> C linker for the host machine: cc ld.bfd 2.34 >> C++ compiler for the host machine: c++ (gcc 9.3.0 "c++ (Ubuntu >> 9.3.0-10ubuntu2) 9.3.0") >> C++ linker for the host machine: c++ ld.bfd 2.34 > Could it be that the android option is set to false in your build ? > Here's what I get with gcc 9.3.0. > > [24/172] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' > FAILED: src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o > g++-9.3.0 -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -Iinclude/libcamera -I/usr/include/libexif -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' -c ../../src/android/jpeg/exif.cpp > In file included from ../../src/android/jpeg/exif.cpp:8: > ../../src/android/jpeg/exif.h:32:6: error: attribute ignored [-Werror=attributes] > 32 | int [[nodiscard]] generate(); > | ^ > ../../src/android/jpeg/exif.h:32:6: note: an attribute that appertains to a type-specifier is ignored > ../../src/android/jpeg/exif.cpp:218:6: error: no declaration matches ‘void Exif::generate()’ > 218 | void Exif::generate() > | ^~~~ > In file included from ../../src/android/jpeg/exif.cpp:8: > ../../src/android/jpeg/exif.h:32:21: note: candidate is: ‘int Exif::generate()’ > 32 | int [[nodiscard]] generate(); > | ^~~~~~~~ > ../../src/android/jpeg/exif.h:18:7: note: ‘class Exif’ defined here > 18 | class Exif > | ^~~~ > cc1plus: all warnings being treated as errors Oh damn! In my ./build-libcamera.sh script, The -Dandroid=true had an spell error- -Danroid=true. The build did warn in the logs(now that I can read/see) but it didn't "abort" the build which made me think that it passed, heh! I should pay more attention to meson build logs. :-( Apologies for the noise. >>>> Didn't we add a [[nodiscard]] to this call? >>>> >>>> Although - if it does fail to generate, I think we'd still expect to >>>> call into the encoder->encode() and I think the exif.data() would be >>>> nullptr,0, (which is what we want), as the encoder checks the size to >>>> determine if it should be added or not. >>>> >>>>> + >>>>> + 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 9d8d9bc..5def7e3 100644 >>>>> --- a/src/android/jpeg/exif.cpp >>>>> +++ b/src/android/jpeg/exif.cpp >>>>> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str >>>>> exif_entry_unref(entry); >>>>> } >>>>> >>>>> -int Exif::generate() >>>>> +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 = 1; >>>>> + switch (orientation) { >>>>> + case 90: >>>>> + value = 6; >>>>> + break; >>>>> + case 180: >>>>> + value = 3; >>>>> + break; >>>>> + case 270: >>>>> + value = 8; >>>>> + break; >>>>> + } >>>>> + >>>>> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); >>>>> +} >>>>> + >>>>> +void Exif::generate() >>>>> { >>>>> if (exifData_) { >>>>> free(exifData_); >>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h >>>>> index 8fb8ffd..6113ca6 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_ }; } >>>>> int [[nodiscard]] generate(); >>>>>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index de6f86f..54ca9c6 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,22 @@ 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)); + 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/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 9d8d9bc..5def7e3 100644 --- a/src/android/jpeg/exif.cpp +++ b/src/android/jpeg/exif.cpp @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str exif_entry_unref(entry); } -int Exif::generate() +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 = 1; + switch (orientation) { + case 90: + value = 6; + break; + case 180: + value = 3; + break; + case 270: + value = 8; + break; + } + + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value); +} + +void Exif::generate() { if (exifData_) { free(exifData_); diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h index 8fb8ffd..6113ca6 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_ }; } int [[nodiscard]] generate();