[libcamera-devel,v6,1/1] Fix Android adaptor thumbnail buffer lifetime
diff mbox series

Message ID 20220720095812.2380218-2-chenghaoyang@google.com
State Superseded
Headers show
Series
  • Android adaptor thumbnail buffer lifetime issue
Related show

Commit Message

Harvey Yang July 20, 2022, 9:58 a.m. UTC 
Previously the thumbnail buffer is destructed before even being used in
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@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(-)

Comments

Umang Jain July 21, 2022, 5:44 p.m. UTC | #1 
Hello Harvey,

Thank you for the patch.

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!

> 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@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().
>    */
> -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.


> +	data_->size = thumbnail_raw_.size();
>   
>   	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 ;-)

Once reviewed by someone else, I 'll handle the merge so,

Reviewed-by: Umang Jain <umang.jain@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);
Laurent Pinchart July 21, 2022, 7:30 p.m. UTC | #2 
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@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@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);
Harvey Yang July 22, 2022, 7:07 a.m. UTC | #3 
Thanks Umang & Laurent!

Followed most of your suggestions and sent the PATCH v2. Please check :)

BR,
Harvey

On Fri, Jul 22, 2022 at 3:30 AM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> 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@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@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
>

Patch
diff mbox series 

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().
  */
-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());
+	data_->size = thumbnail_raw_.size();
 
 	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_;
 };
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);