[libcamera-devel] [PATCH v2 3/3] android: jpeg: post_processor_jpeg: Embed thumbnail into Exif metadata
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 28 02:29:54 CET 2020
Hi Umang,
Thank you for the patch.
On Wed, Oct 28, 2020 at 02:54:47AM +0530, Umang Jain wrote:
> Embed a Jpeg-encoded thumbnail into Exif metadata using the Thumbnailer
> class that got introduced.
>
> Introduce a helper function in Exif class for setting the thumbnail
> data. Set the EXIF_TAG_COMPRESSION to '6' to denote that the thumbnail
> is jpeg-compressed, as mentioned in Exif v2.31.
>
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
> src/android/jpeg/exif.cpp | 24 ++++++++++++++++-
> src/android/jpeg/exif.h | 2 ++
> src/android/jpeg/post_processor_jpeg.cpp | 34 ++++++++++++++++++++++++
> src/android/jpeg/post_processor_jpeg.h | 8 +++++-
> 4 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index d21534a..6ac52c6 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -75,8 +75,16 @@ Exif::~Exif()
> if (exifData_)
> free(exifData_);
>
> - if (data_)
> + if (data_) {
> + /*
> + * Reset thumbnail data to avoid getting double-freed by
> + * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> + */
> + data_->data = nullptr;
> + data_->size = 0;
> +
> exif_data_unref(data_);
> + }
>
> if (mem_)
> exif_mem_unref(mem_);
> @@ -268,6 +276,20 @@ void Exif::setOrientation(int orientation)
> setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> }
>
> +/*
> + * The thumbnail data should remain valid until the Exif object is destroyed.
> + * Failing to do so, might result in no thumbnail data being set even after a
> + * call to Exif::setThumbnail().
> + */
> +void Exif::setThumbnail(Span<const unsigned char> thumbnail,
> + uint16_t compression)
> +{
> + data_->data = const_cast<unsigned char *>(thumbnail.data());
> + data_->size = thumbnail.size();
> +
> + setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
> +}
> +
> [[nodiscard]] int Exif::generate()
> {
> if (exifData_) {
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index 12c27b6..6987b31 100644
> --- a/src/android/jpeg/exif.h
> +++ b/src/android/jpeg/exif.h
> @@ -26,6 +26,8 @@ public:
>
> void setOrientation(int orientation);
> void setSize(const libcamera::Size &size);
> + void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> + uint16_t compression);
> void setTimestamp(time_t timestamp);
>
> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index c56f1b2..a0db793 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -39,11 +39,41 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> }
>
> streamSize_ = outCfg.size;
> +
> + thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> + StreamConfiguration thCfg = inCfg;
> + thCfg.size = thumbnailer_.size();
> + if (thumbnailEncoder_.configure(thCfg) != 0) {
> + LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> + return -EINVAL;
> + }
> +
> encoder_ = std::make_unique<EncoderLibJpeg>();
>
> return encoder_->configure(inCfg);
> }
>
> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> + std::vector<unsigned char> *thumbnail)
> +{
> + /* Stores the raw scaled-down thumbnail bytes. */
> + std::vector<unsigned char> rawThumbnail;
> +
> + thumbnailer_.createThumbnail(source, &rawThumbnail);
> +
> + if (!rawThumbnail.empty()) {
> + thumbnail->resize(rawThumbnail.size());
This will zero the contents of the vector, which isn't required. Let's
leave it as-is for now, but record the issue:
/*
* \todo Avoid value-initialization of all elements of the
* vector.
*/
> +
> + int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,
> + *thumbnail, { });
> + thumbnail->resize(jpeg_size);
> +
> + LOG(JPEG, Debug)
> + << "Thumbnail compress returned "
> + << jpeg_size << " bytes";
> + }
> +}
> +
> int PostProcessorJpeg::process(const FrameBuffer &source,
> Span<uint8_t> destination,
> CameraMetadata *metadata)
> @@ -64,6 +94,10 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> * second, it is good enough.
> */
> exif.setTimestamp(std::time(nullptr));
> + std::vector<unsigned char> thumbnail;
> + generateThumbnail(source, &thumbnail);
I think we could return the vector from generateThumbnail() instead of
passing it from the caller, but that's also not something that needs to
be addressed now.
> + if(!thumbnail.empty())
> + exif.setThumbnail(thumbnail, 6);
The magic value isn't very nice. How about turning the parameter to a
'bool compressed', or, better, to an enum declared in the Exif class
enum class Compression {
None = 1,
JPEG = 6,
};
and turning setThumbnail() into
void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
Compression compression);
Apart from that the patch looks good to me.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
I'll let Kieran have a look too, but I think these small issues could be
fixed while applying (unless he would prefer you to test them first).
> if (exif.generate() != 0)
> LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 3706cec..5afa831 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -8,12 +8,13 @@
> #define __ANDROID_POST_PROCESSOR_JPEG_H__
>
> #include "../post_processor.h"
> +#include "encoder_libjpeg.h"
> +#include "thumbnailer.h"
>
> #include <libcamera/geometry.h>
>
> #include "libcamera/internal/buffer.h"
>
> -class Encoder;
> class CameraDevice;
>
> class PostProcessorJpeg : public PostProcessor
> @@ -28,9 +29,14 @@ public:
> CameraMetadata *metadata) override;
>
> private:
> + void generateThumbnail(const libcamera::FrameBuffer &source,
> + std::vector<unsigned char> *thumbnail);
> +
> CameraDevice *const cameraDevice_;
> std::unique_ptr<Encoder> encoder_;
> libcamera::Size streamSize_;
> + EncoderLibJpeg thumbnailEncoder_;
> + Thumbnailer thumbnailer_;
> };
>
> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list