| Message ID | 20200824204646.16866-4-email@uajain.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 24/08/2020 21:46, Umang Jain wrote: > Support a initial set of EXIF metadata tags namely: > Make, Model, Height, Width, Orientation and Timestamp. > Each tag is set by a convenient high level helper API > defined in exif.h. > > Signed-off-by: Umang Jain <email@uajain.com> > --- > src/android/camera_device.cpp | 8 +++++++ > src/android/jpeg/encoder_libjpeg.cpp | 12 ++++++++++ > src/android/jpeg/exif.cpp | 34 ++++++++++++++++++++++++++++ > src/android/jpeg/exif.h | 8 +++++++ I'd merge the helpers directly into the Exif object creation patch (and take authorship on that if you like), or as an additional patch on it's own after. > 4 files changed, 62 insertions(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index bc5690e..fcf378a 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1435,6 +1435,14 @@ void CameraDevice::requestComplete(Request *request) > } > > Exif exif; > + /* > + * \todo Discuss setMake String, maybe vendor ID? > + * KB suggested to leave it to "libcamera" for now. > + * setModel should use the 'model' property of the camera. > + */ I think ultimately this will indeed need to come from some sort of vendor tag. But I don't think we have those yet, so libcamera should be fine for now ;-) > + exif.setMake("Libcamera"); libcamera always has a lowercase 'l'. > + exif.setModel("cameraModel"); Aha, are the patches for setting the model not integrated yet ? That would indeed mean it's a todo for now, but hopefully that will be usable really soon, so I hope that can be done before these patches get in. > + exif.setOrientation(orientation_); > > int jpeg_size = encoder->encode(buffer, mapped.maps()[0], &exif); > if (jpeg_size < 0) { > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index 0cd93b6..f84def9 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -213,7 +213,19 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > else > compressRGB(&frame); > > + exif->setWidth(compress_.image_width); > + exif->setHeight(compress_.image_height); > + exif->setTimestamp(source->metadata().timestamp); I hope all three of those could be obtained/determined in the layer above, so we don't need to pass in the whole exif object, just the generated span, > + > + Span<uint8_t> exif_data = exif->generate(); > + So if you can do that in the camera_device.cpp, then we > jpeg_finish_compress(&compress_); > > + if (exif->size()) > + /* Store Exif data in the JPEG_APP1 data block. */ > + jpeg_write_marker(&compress_, JPEG_APP0 + 1, > + static_cast<const JOCTET *>(exif_data.data()), > + exif_data.size()); > + > return size; > } > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > index f6a9f5c..b7591d5 100644 > --- a/src/android/jpeg/exif.cpp > +++ b/src/android/jpeg/exif.cpp > @@ -160,6 +160,40 @@ int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::stri > return 0; > } > > +int Exif::setHeight(uint16_t height) > +{ > + setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, height); > + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, height); > + > + return 0; > +} > + > +int Exif::setWidth(uint16_t width) > +{ > + setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, width); > + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, width); > + > + return 0; > +} > + > +int Exif::setTimestamp(const time_t timestamp) > +{ > + std::string ts(std::ctime(×tamp)); > + int ret = setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); > + if(ret < 0) > + return ret; > + > + ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); > + if(ret < 0) > + return ret; > + > + ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); > + if(ret < 0) > + return ret; > + > + return ret; > +} > + > Span<uint8_t> Exif::generate() > { > if (exif_data_) { > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > index 7df83c7..6a8f5f5 100644 > --- a/src/android/jpeg/exif.h > +++ b/src/android/jpeg/exif.h > @@ -11,6 +11,7 @@ > > #include <libcamera/span.h> > > +#include <ctime> > #include <string> > > class Exif > @@ -24,6 +25,13 @@ public: > int setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item); > int setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator); > > + int setHeight(uint16_t height); > + int setMake(const std::string &make) { return setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); } > + int setModel(const std::string &model) { return setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); } > + int setOrientation(int orientation) { return setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, orientation); } Even if they're short, I think these implementations would probably be better in the .cpp so that all helpers are in the same place. Orientation will potentially be more complex too, as I think there will need to be a translation between the libcamera orientation value, and the Exif orientation values. I remember seeing some helpful comments in the USB HAL in CrOS/Camera2 for that. > + int setTimestamp(const time_t timestamp); > + int setWidth(uint16_t width); I'd group by feature over sorting alphabetically, and keep setWidth, setHeight together. -- Kieran > + > libcamera::Span<uint8_t> generate(); > unsigned char *data() const { return exif_data_; } > unsigned int size() const { return size_; } >
Hi Kieran, On Mon, Aug 24, 2020 at 10:14:40PM +0100, Kieran Bingham wrote: > On 24/08/2020 21:46, Umang Jain wrote: > > Support a initial set of EXIF metadata tags namely: > > Make, Model, Height, Width, Orientation and Timestamp. > > Each tag is set by a convenient high level helper API > > defined in exif.h. > > > > Signed-off-by: Umang Jain <email@uajain.com> > > --- > > src/android/camera_device.cpp | 8 +++++++ > > src/android/jpeg/encoder_libjpeg.cpp | 12 ++++++++++ > > src/android/jpeg/exif.cpp | 34 ++++++++++++++++++++++++++++ > > src/android/jpeg/exif.h | 8 +++++++ > > I'd merge the helpers directly into the Exif object creation patch (and > take authorship on that if you like), or as an additional patch on it's > own after. > > > 4 files changed, 62 insertions(+) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index bc5690e..fcf378a 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1435,6 +1435,14 @@ void CameraDevice::requestComplete(Request *request) > > } > > > > Exif exif; > > + /* > > + * \todo Discuss setMake String, maybe vendor ID? > > + * KB suggested to leave it to "libcamera" for now. > > + * setModel should use the 'model' property of the camera. > > + */ > > I think ultimately this will indeed need to come from some sort of > vendor tag. But I don't think we have those yet, so libcamera should be > fine for now ;-) > > > + exif.setMake("Libcamera"); > > libcamera always has a lowercase 'l'. > > > + exif.setModel("cameraModel"); > > Aha, are the patches for setting the model not integrated yet ? > > That would indeed mean it's a todo for now, but hopefully that will be > usable really soon, so I hope that can be done before these patches get in. Wouldn't the maker and vendor we expect here would be along the lines of "Jolla" / "TheFirstOne" or "Google" / "Pixel 4" ? I think we can thus hardcode them here, until we add support for a platform config/data file. There's probably no point in filling these with the model reported by the pipeline handler, as that would just be extra code that will have to be replaced anyway. > > + exif.setOrientation(orientation_); > > > > int jpeg_size = encoder->encode(buffer, mapped.maps()[0], &exif); > > if (jpeg_size < 0) { > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > > index 0cd93b6..f84def9 100644 > > --- a/src/android/jpeg/encoder_libjpeg.cpp > > +++ b/src/android/jpeg/encoder_libjpeg.cpp > > @@ -213,7 +213,19 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, > > else > > compressRGB(&frame); > > > > + exif->setWidth(compress_.image_width); > > + exif->setHeight(compress_.image_height); > > + exif->setTimestamp(source->metadata().timestamp); > > I hope all three of those could be obtained/determined in the layer > above, so we don't need to pass in the whole exif object, just the > generated span, > > > + > > + Span<uint8_t> exif_data = exif->generate(); > > + > > So if you can do that in the camera_device.cpp, then we > > > jpeg_finish_compress(&compress_); > > > > + if (exif->size()) > > + /* Store Exif data in the JPEG_APP1 data block. */ > > + jpeg_write_marker(&compress_, JPEG_APP0 + 1, > > + static_cast<const JOCTET *>(exif_data.data()), > > + exif_data.size()); > > + > > return size; > > } > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > > index f6a9f5c..b7591d5 100644 > > --- a/src/android/jpeg/exif.cpp > > +++ b/src/android/jpeg/exif.cpp > > @@ -160,6 +160,40 @@ int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::stri > > return 0; > > } > > > > +int Exif::setHeight(uint16_t height) > > +{ > > + setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, height); > > + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, height); > > + > > + return 0; > > +} > > + > > +int Exif::setWidth(uint16_t width) > > +{ > > + setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, width); > > + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, width); > > + > > + return 0; > > +} > > + > > +int Exif::setTimestamp(const time_t timestamp) > > +{ > > + std::string ts(std::ctime(×tamp)); > > + int ret = setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); > > + if(ret < 0) > > + return ret; > > + > > + ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); > > + if(ret < 0) > > + return ret; > > + > > + ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); > > + if(ret < 0) > > + return ret; > > + > > + return ret; > > +} > > + > > Span<uint8_t> Exif::generate() > > { > > if (exif_data_) { > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > > index 7df83c7..6a8f5f5 100644 > > --- a/src/android/jpeg/exif.h > > +++ b/src/android/jpeg/exif.h > > @@ -11,6 +11,7 @@ > > > > #include <libcamera/span.h> > > > > +#include <ctime> > > #include <string> > > > > class Exif > > @@ -24,6 +25,13 @@ public: > > int setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item); > > int setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator); > > > > + int setHeight(uint16_t height); > > + int setMake(const std::string &make) { return setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); } > > + int setModel(const std::string &model) { return setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); } > > + int setOrientation(int orientation) { return setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, orientation); } > > Even if they're short, I think these implementations would probably be > better in the .cpp so that all helpers are in the same place. > > Orientation will potentially be more complex too, as I think there will > need to be a translation between the libcamera orientation value, and > the Exif orientation values. > > I remember seeing some helpful comments in the USB HAL in CrOS/Camera2 > for that. > > > + int setTimestamp(const time_t timestamp); > > + int setWidth(uint16_t width); > > I'd group by feature over sorting alphabetically, and keep setWidth, > setHeight together. > > > + > > libcamera::Span<uint8_t> generate(); > > unsigned char *data() const { return exif_data_; } > > unsigned int size() const { return size_; }
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index bc5690e..fcf378a 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1435,6 +1435,14 @@ void CameraDevice::requestComplete(Request *request) } Exif exif; + /* + * \todo Discuss setMake String, maybe vendor ID? + * KB suggested to leave it to "libcamera" for now. + * setModel should use the 'model' property of the camera. + */ + exif.setMake("Libcamera"); + exif.setModel("cameraModel"); + exif.setOrientation(orientation_); int jpeg_size = encoder->encode(buffer, mapped.maps()[0], &exif); if (jpeg_size < 0) { diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index 0cd93b6..f84def9 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -213,7 +213,19 @@ int EncoderLibJpeg::encode(const FrameBuffer *source, else compressRGB(&frame); + exif->setWidth(compress_.image_width); + exif->setHeight(compress_.image_height); + exif->setTimestamp(source->metadata().timestamp); + + Span<uint8_t> exif_data = exif->generate(); + jpeg_finish_compress(&compress_); + if (exif->size()) + /* Store Exif data in the JPEG_APP1 data block. */ + jpeg_write_marker(&compress_, JPEG_APP0 + 1, + static_cast<const JOCTET *>(exif_data.data()), + exif_data.size()); + return size; } diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp index f6a9f5c..b7591d5 100644 --- a/src/android/jpeg/exif.cpp +++ b/src/android/jpeg/exif.cpp @@ -160,6 +160,40 @@ int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::stri return 0; } +int Exif::setHeight(uint16_t height) +{ + setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, height); + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, height); + + return 0; +} + +int Exif::setWidth(uint16_t width) +{ + setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, width); + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, width); + + return 0; +} + +int Exif::setTimestamp(const time_t timestamp) +{ + std::string ts(std::ctime(×tamp)); + int ret = setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts); + if(ret < 0) + return ret; + + ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts); + if(ret < 0) + return ret; + + ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts); + if(ret < 0) + return ret; + + return ret; +} + Span<uint8_t> Exif::generate() { if (exif_data_) { diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h index 7df83c7..6a8f5f5 100644 --- a/src/android/jpeg/exif.h +++ b/src/android/jpeg/exif.h @@ -11,6 +11,7 @@ #include <libcamera/span.h> +#include <ctime> #include <string> class Exif @@ -24,6 +25,13 @@ public: int setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item); int setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator); + int setHeight(uint16_t height); + int setMake(const std::string &make) { return setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); } + int setModel(const std::string &model) { return setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); } + int setOrientation(int orientation) { return setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, orientation); } + int setTimestamp(const time_t timestamp); + int setWidth(uint16_t width); + libcamera::Span<uint8_t> generate(); unsigned char *data() const { return exif_data_; } unsigned int size() const { return size_; }
Support a initial set of EXIF metadata tags namely: Make, Model, Height, Width, Orientation and Timestamp. Each tag is set by a convenient high level helper API defined in exif.h. Signed-off-by: Umang Jain <email@uajain.com> --- src/android/camera_device.cpp | 8 +++++++ src/android/jpeg/encoder_libjpeg.cpp | 12 ++++++++++ src/android/jpeg/exif.cpp | 34 ++++++++++++++++++++++++++++ src/android/jpeg/exif.h | 8 +++++++ 4 files changed, 62 insertions(+)