[libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail buffer lifetime

[Laurent Pinchart]

Hello,

On Thu, Jul 21, 2022 at 11:14:16PM +0530, Umang Jain via libcamera-devel wrote:
> On 7/20/22 15:28, Harvey Yang via libcamera-devel wrote:
> > Previously the thumbnail buffer is destructed before even being used in
> 
> Great catch, I think this was the reason for flaky failure for 
> Camera3SimpleStillCaptureTest.JpegExifTest as mentioned!

Oh indeed ! Nice !

> > Exif. This patch moves the buffer into class Exif, so that the developer
> > won't need to worry about its lifetime.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >   src/android/jpeg/exif.cpp                | 10 ++++++----
> >   src/android/jpeg/exif.h                  | 10 ++++++++--
> >   src/android/jpeg/post_processor_jpeg.cpp |  6 ++----
> >   3 files changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > index 3220b458..584a29d0 100644
> > --- a/src/android/jpeg/exif.cpp
> > +++ b/src/android/jpeg/exif.cpp
> > @@ -435,11 +435,13 @@ void Exif::setOrientation(int orientation)
> >    * Failing to do so, might result in no thumbnail data being set even after a
> >    * call to Exif::setThumbnail().
> >    */

Note that the issue is documented here. Clearly that wasn't enough, so
the API isn't good and needs to be changed. The comment can thus be
dropped.

> > -void Exif::setThumbnail(Span<const unsigned char> thumbnail,
> > -			Compression compression)
> > +void Exif::setThumbnail(Compression compression)
> >   {
> > -	data_->data = const_cast<unsigned char *>(thumbnail.data());
> > -	data_->size = thumbnail.size();
> > +	if (thumbnail_raw_.empty())
> > +		return;
> > +
> > +	data_->data = const_cast<unsigned char *>(thumbnail_raw_.data());
> 
> I think we can do away with the const_cast here? Might not be compatible 
> with previous C++ versions?
> 
> I quickly compile tested
> 
> -       data_->data = const_cast<unsigned char *>(thumbnail_raw_.data());
> +       data_->data = thumbnail_raw_.data();
> 
> on clang version 12.0.1 and gcc (GCC) 11.0.1 which seems OK.

There should be no need for a const_cast, indeed.

> > +	data_->size = thumbnail_raw_.size();

I would do this a bit differently, to avoid the getThumbnailRaw(), which
really creates an interdependency between the Exif class and the
PostProcessorJpeg class.

void Exif::setThumbnail(std::vector<uint8_t> &&data, Compression compression)
{
	thumbnail_raw_ = std::move(data);

	data_->data = thumbnail_raw_.data();
	data_->size = thumbnail_raw_.size();

   	setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
}

The only change to the caller compared to the current code would then be

-				exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
+				exif.setThumbnail(std::move(thumbnail),
+						  Exif::Compression::JPEG);

> >   
> >   	setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
> >   }
> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > index 2ff8fb78..4784496b 100644
> > --- a/src/android/jpeg/exif.h
> > +++ b/src/android/jpeg/exif.h
> > @@ -10,6 +10,7 @@
> >   #include <chrono>
> >   #include <string>
> >   #include <time.h>
> > +#include <vector>
> >   
> >   #include <libexif/exif-data.h>
> >   
> > @@ -60,8 +61,7 @@ public:
> >   
> >   	void setOrientation(int orientation);
> >   	void setSize(const libcamera::Size &size);
> > -	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> > -			  Compression compression);
> > +	void setThumbnail(Compression compression);
> >   	void setTimestamp(time_t timestamp, std::chrono::milliseconds msec);
> >   
> >   	void setGPSDateTimestamp(time_t timestamp);
> > @@ -78,6 +78,10 @@ public:
> >   	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> >   	[[nodiscard]] int generate();
> >   
> > +	std::vector<unsigned char>& getThumbnailRaw() {
> > +		return thumbnail_raw_;
> > +	}
> > +
> >   private:
> >   	ExifEntry *createEntry(ExifIfd ifd, ExifTag tag);
> >   	ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > @@ -106,4 +110,6 @@ private:
> >   
> >   	unsigned char *exifData_;
> >   	unsigned int size_;
> > +
> > +	std::vector<unsigned char> thumbnail_raw_;
> 
> CamelCase everywhere please ;-)

And I'd call it thumbnailData_, or just thumbnail_.

> Once reviewed by someone else, I 'll handle the merge so,
> 
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> 
> >   };
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index d72ebc3c..506b00b9 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -163,10 +163,8 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
> >   		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, quality);
> >   
> >   		if (thumbnailSize != Size(0, 0)) {
> > -			std::vector<unsigned char> thumbnail;
> > -			generateThumbnail(source, thumbnailSize, quality, &thumbnail);
> > -			if (!thumbnail.empty())
> > -				exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
> > +			generateThumbnail(source, thumbnailSize, quality, &exif.getThumbnailRaw());
> > +			exif.setThumbnail(Exif::Compression::JPEG);
> >   		}
> >   
> >   		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);

-- 
Regards,

Laurent Pinchart