<div dir="ltr">Thanks Umang & Laurent!<div><br></div><div>Followed most of your suggestions and sent the PATCH v2. Please check :)</div><div><br></div><div>BR,</div><div>Harvey</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 22, 2022 at 3:30 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
On Thu, Jul 21, 2022 at 11:14:16PM +0530, Umang Jain via libcamera-devel wrote:<br>
> On 7/20/22 15:28, Harvey Yang via libcamera-devel wrote:<br>
> > Previously the thumbnail buffer is destructed before even being used in<br>
> <br>
> Great catch, I think this was the reason for flaky failure for <br>
> Camera3SimpleStillCaptureTest.JpegExifTest as mentioned!<br>
<br>
Oh indeed ! Nice !<br>
<br>
> > Exif. This patch moves the buffer into class Exif, so that the developer<br>
> > won't need to worry about its lifetime.<br>
> ><br>
> > Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > ---<br>
> > src/android/jpeg/exif.cpp | 10 ++++++----<br>
> > src/android/jpeg/exif.h | 10 ++++++++--<br>
> > src/android/jpeg/post_processor_jpeg.cpp | 6 ++----<br>
> > 3 files changed, 16 insertions(+), 10 deletions(-)<br>
> ><br>
> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp<br>
> > index 3220b458..584a29d0 100644<br>
> > --- a/src/android/jpeg/exif.cpp<br>
> > +++ b/src/android/jpeg/exif.cpp<br>
> > @@ -435,11 +435,13 @@ void Exif::setOrientation(int orientation)<br>
> > * Failing to do so, might result in no thumbnail data being set even after a<br>
> > * call to Exif::setThumbnail().<br>
> > */<br>
<br>
Note that the issue is documented here. Clearly that wasn't enough, so<br>
the API isn't good and needs to be changed. The comment can thus be<br>
dropped.<br>
<br>
> > -void Exif::setThumbnail(Span<const unsigned char> thumbnail,<br>
> > - Compression compression)<br>
> > +void Exif::setThumbnail(Compression compression)<br>
> > {<br>
> > - data_->data = const_cast<unsigned char *>(thumbnail.data());<br>
> > - data_->size = thumbnail.size();<br>
> > + if (thumbnail_raw_.empty())<br>
> > + return;<br>
> > +<br>
> > + data_->data = const_cast<unsigned char *>(thumbnail_raw_.data());<br>
> <br>
> I think we can do away with the const_cast here? Might not be compatible <br>
> with previous C++ versions?<br>
> <br>
> I quickly compile tested<br>
> <br>
> - data_->data = const_cast<unsigned char *>(thumbnail_raw_.data());<br>
> + data_->data = thumbnail_raw_.data();<br>
> <br>
> on clang version 12.0.1 and gcc (GCC) 11.0.1 which seems OK.<br>
<br>
There should be no need for a const_cast, indeed.<br>
<br>
> > + data_->size = thumbnail_raw_.size();<br>
<br>
I would do this a bit differently, to avoid the getThumbnailRaw(), which<br>
really creates an interdependency between the Exif class and the<br>
PostProcessorJpeg class.<br>
<br>
void Exif::setThumbnail(std::vector<uint8_t> &&data, Compression compression)<br>
{<br>
thumbnail_raw_ = std::move(data);<br>
<br>
data_->data = thumbnail_raw_.data();<br>
data_->size = thumbnail_raw_.size();<br>
<br>
setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);<br>
}<br>
<br>
The only change to the caller compared to the current code would then be<br>
<br>
- exif.setThumbnail(thumbnail, Exif::Compression::JPEG);<br>
+ exif.setThumbnail(std::move(thumbnail),<br>
+ Exif::Compression::JPEG);<br>
<br>
> > <br>
> > setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);<br>
> > }<br>
> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h<br>
> > index 2ff8fb78..4784496b 100644<br>
> > --- a/src/android/jpeg/exif.h<br>
> > +++ b/src/android/jpeg/exif.h<br>
> > @@ -10,6 +10,7 @@<br>
> > #include <chrono><br>
> > #include <string><br>
> > #include <time.h><br>
> > +#include <vector><br>
> > <br>
> > #include <libexif/exif-data.h><br>
> > <br>
> > @@ -60,8 +61,7 @@ public:<br>
> > <br>
> > void setOrientation(int orientation);<br>
> > void setSize(const libcamera::Size &size);<br>
> > - void setThumbnail(libcamera::Span<const unsigned char> thumbnail,<br>
> > - Compression compression);<br>
> > + void setThumbnail(Compression compression);<br>
> > void setTimestamp(time_t timestamp, std::chrono::milliseconds msec);<br>
> > <br>
> > void setGPSDateTimestamp(time_t timestamp);<br>
> > @@ -78,6 +78,10 @@ public:<br>
> > libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }<br>
> > [[nodiscard]] int generate();<br>
> > <br>
> > + std::vector<unsigned char>& getThumbnailRaw() {<br>
> > + return thumbnail_raw_;<br>
> > + }<br>
> > +<br>
> > private:<br>
> > ExifEntry *createEntry(ExifIfd ifd, ExifTag tag);<br>
> > ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,<br>
> > @@ -106,4 +110,6 @@ private:<br>
> > <br>
> > unsigned char *exifData_;<br>
> > unsigned int size_;<br>
> > +<br>
> > + std::vector<unsigned char> thumbnail_raw_;<br>
> <br>
> CamelCase everywhere please ;-)<br>
<br>
And I'd call it thumbnailData_, or just thumbnail_.<br>
<br>
> Once reviewed by someone else, I 'll handle the merge so,<br>
> <br>
> Reviewed-by: Umang Jain <<a href="mailto:umang.jain@ideasonboard.com" target="_blank">umang.jain@ideasonboard.com</a>><br>
> <br>
> > };<br>
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp<br>
> > index d72ebc3c..506b00b9 100644<br>
> > --- a/src/android/jpeg/post_processor_jpeg.cpp<br>
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp<br>
> > @@ -163,10 +163,8 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu<br>
> > resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, quality);<br>
> > <br>
> > if (thumbnailSize != Size(0, 0)) {<br>
> > - std::vector<unsigned char> thumbnail;<br>
> > - generateThumbnail(source, thumbnailSize, quality, &thumbnail);<br>
> > - if (!thumbnail.empty())<br>
> > - exif.setThumbnail(thumbnail, Exif::Compression::JPEG);<br>
> > + generateThumbnail(source, thumbnailSize, quality, &exif.getThumbnailRaw());<br>
> > + exif.setThumbnail(Exif::Compression::JPEG);<br>
> > }<br>
> > <br>
> > resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>