[{"id":24034,"web_url":"https://patchwork.libcamera.org/comment/24034/","msgid":"","date":"2022-07-21T17:44:16","subject":"Re: [libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail\n\tbuffer lifetime","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello Harvey,\n\nThank you for the patch.\n\nOn 7/20/22 15:28, Harvey Yang via libcamera-devel wrote:\n> Previously the thumbnail buffer is destructed before even being used in\n\n\nGreat catch, I think this was the reason for flaky failure for \nCamera3SimpleStillCaptureTest.JpegExifTest as mentioned!\n\n> Exif. This patch moves the buffer into class Exif, so that the developer\n> won't need to worry about its lifetime.\n>\n> Signed-off-by: Harvey Yang \n> ---\n> src/android/jpeg/exif.cpp | 10 ++++++----\n> src/android/jpeg/exif.h | 10 ++++++++--\n> src/android/jpeg/post_processor_jpeg.cpp | 6 ++----\n> 3 files changed, 16 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 3220b458..584a29d0 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -435,11 +435,13 @@ void Exif::setOrientation(int orientation)\n> * Failing to do so, might result in no thumbnail data being set even after a\n> * call to Exif::setThumbnail().\n> */\n> -void Exif::setThumbnail(Span thumbnail,\n> -\t\t\tCompression compression)\n> +void Exif::setThumbnail(Compression compression)\n> {\n> -\tdata_->data = const_cast(thumbnail.data());\n> -\tdata_->size = thumbnail.size();\n> +\tif (thumbnail_raw_.empty())\n> +\t\treturn;\n> +\n> +\tdata_->data = const_cast(thumbnail_raw_.data());\n\n\nI think we can do away with the const_cast here? Might not be compatible \nwith previous C++ versions?\n\nI quickly compile tested\n\n-       data_->data = const_cast(thumbnail_raw_.data());\n+       data_->data = thumbnail_raw_.data();\n\non clang version 12.0.1 and gcc (GCC) 11.0.1 which seems OK.\n\n\n> +\tdata_->size = thumbnail_raw_.size();\n> \n> \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> }\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> index 2ff8fb78..4784496b 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> @@ -60,8 +61,7 @@ public:\n> \n> \tvoid setOrientation(int orientation);\n> \tvoid setSize(const libcamera::Size &size);\n> -\tvoid setThumbnail(libcamera::Span thumbnail,\n> -\t\t\t Compression compression);\n> +\tvoid setThumbnail(Compression compression);\n> \tvoid setTimestamp(time_t timestamp, std::chrono::milliseconds msec);\n> \n> \tvoid setGPSDateTimestamp(time_t timestamp);\n> @@ -78,6 +78,10 @@ public:\n> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> \t[[nodiscard]] int generate();\n> \n> +\tstd::vector& getThumbnailRaw() {\n> +\t\treturn thumbnail_raw_;\n> +\t}\n> +\n> private:\n> \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag);\n> \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> @@ -106,4 +110,6 @@ private:\n> \n> \tunsigned char *exifData_;\n> \tunsigned int size_;\n> +\n> +\tstd::vector thumbnail_raw_;\n\n\nCamelCase everywhere please ;-)\n\nOnce reviewed by someone else, I 'll handle the merge so,\n\nReviewed-by: Umang Jain \n\n> };\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index d72ebc3c..506b00b9 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -163,10 +163,8 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> \t\tresultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, quality);\n> \n> \t\tif (thumbnailSize != Size(0, 0)) {\n> -\t\t\tstd::vector thumbnail;\n> -\t\t\tgenerateThumbnail(source, thumbnailSize, quality, &thumbnail);\n> -\t\t\tif (!thumbnail.empty())\n> -\t\t\t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n> +\t\t\tgenerateThumbnail(source, thumbnailSize, quality, &exif.getThumbnailRaw());\n> +\t\t\texif.setThumbnail(Exif::Compression::JPEG);\n> \t\t}\n> \n> \t\tresultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 44786BE173\n\tfor ;\n\tThu, 21 Jul 2022 17:44:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E8206330F;\n\tThu, 21 Jul 2022 19:44:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F481601B8\n\tfor ;\n\tThu, 21 Jul 2022 19:44:23 +0200 (CEST)","from [IPV6:2401:4900:1f3f:a2d:c6dd:7a7c:3d3c:dbb9] (unknown\n\t[IPv6:2401:4900:1f3f:a2d:c6dd:7a7c:3d3c:dbb9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A376496;\n\tThu, 21 Jul 2022 19:44:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658425464;\n\tbh=orMjofREX9oCmbtVuSEJlNtzmC4lH3FCEj0p5nQGJto=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ReTYn20c6/u/e/ln4k+cdHrPKiatuedXLVfyYJlkac48gynxjBmn26YAbU8CQ+Il5\n\t7JkOcOWkoGYEH/grQzarpchDXSRVpkL2Uq6INB2kxKxB3vIA0ffq2STE1CYe0pKCk3\n\tp1SEYODkuN1SA2uzuOLXr+ERWGMdWIQL6awATZB5HgIW5ar/5LEKPsTxubi5+cYG2M\n\tMpnqnkiKUDzF6KMAZMFRpa4UCqc+vly0WC6LAft10o69lW6tJaeOZQwr82FfJ4aHkQ\n\tIzAGJ+77Mgzu+lkOOPmTiYWIJk6rut/K4r7byEI4Q1vdpZtZ9G0PnPAph3NUXJMRMc\n\t19nAnIOfdr0QA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658425462;\n\tbh=orMjofREX9oCmbtVuSEJlNtzmC4lH3FCEj0p5nQGJto=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Vzb3h0Qt3zYnZFHHaeV5PfA3ADWFKYvcXfW0hccZlG//0RTSCVARBoYlIzDQZQPSo\n\tBJrhkQKiwqoP8z7ackAimS4y4FyVWPQG9lFG/kY2yoT8adx5h/PvleF1/Gqss0rScn\n\twR5FqJ59sAOJNi2mg+EdjZlWFXD+0pdo1CVHPHm4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Vzb3h0Qt\"; dkim-atps=neutral","Message-ID":"","Date":"Thu, 21 Jul 2022 23:14:16 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220720095812.2380218-1-chenghaoyang@google.com>\n\t<20220720095812.2380218-2-chenghaoyang@google.com>","In-Reply-To":"<20220720095812.2380218-2-chenghaoyang@google.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail\n\tbuffer lifetime","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"Harvey Yang ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24036,"web_url":"https://patchwork.libcamera.org/comment/24036/","msgid":"","date":"2022-07-21T19:30:40","subject":"Re: [libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail\n\tbuffer lifetime","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Jul 21, 2022 at 11:14:16PM +0530, Umang Jain via libcamera-devel wrote:\n> On 7/20/22 15:28, Harvey Yang via libcamera-devel wrote:\n> > Previously the thumbnail buffer is destructed before even being used in\n> \n> Great catch, I think this was the reason for flaky failure for \n> Camera3SimpleStillCaptureTest.JpegExifTest as mentioned!\n\nOh indeed ! Nice !\n\n> > Exif. This patch moves the buffer into class Exif, so that the developer\n> > won't need to worry about its lifetime.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/android/jpeg/exif.cpp | 10 ++++++----\n> > src/android/jpeg/exif.h | 10 ++++++++--\n> > src/android/jpeg/post_processor_jpeg.cpp | 6 ++----\n> > 3 files changed, 16 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > index 3220b458..584a29d0 100644\n> > --- a/src/android/jpeg/exif.cpp\n> > +++ b/src/android/jpeg/exif.cpp\n> > @@ -435,11 +435,13 @@ void Exif::setOrientation(int orientation)\n> > * Failing to do so, might result in no thumbnail data being set even after a\n> > * call to Exif::setThumbnail().\n> > */\n\nNote that the issue is documented here. Clearly that wasn't enough, so\nthe API isn't good and needs to be changed. The comment can thus be\ndropped.\n\n> > -void Exif::setThumbnail(Span thumbnail,\n> > -\t\t\tCompression compression)\n> > +void Exif::setThumbnail(Compression compression)\n> > {\n> > -\tdata_->data = const_cast(thumbnail.data());\n> > -\tdata_->size = thumbnail.size();\n> > +\tif (thumbnail_raw_.empty())\n> > +\t\treturn;\n> > +\n> > +\tdata_->data = const_cast(thumbnail_raw_.data());\n> \n> I think we can do away with the const_cast here? Might not be compatible \n> with previous C++ versions?\n> \n> I quickly compile tested\n> \n> -       data_->data = const_cast(thumbnail_raw_.data());\n> +       data_->data = thumbnail_raw_.data();\n> \n> on clang version 12.0.1 and gcc (GCC) 11.0.1 which seems OK.\n\nThere should be no need for a const_cast, indeed.\n\n> > +\tdata_->size = thumbnail_raw_.size();\n\nI would do this a bit differently, to avoid the getThumbnailRaw(), which\nreally creates an interdependency between the Exif class and the\nPostProcessorJpeg class.\n\nvoid Exif::setThumbnail(std::vector &&data, Compression compression)\n{\n\tthumbnail_raw_ = std::move(data);\n\n\tdata_->data = thumbnail_raw_.data();\n\tdata_->size = thumbnail_raw_.size();\n\n \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n}\n\nThe only change to the caller compared to the current code would then be\n\n-\t\t\t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n+\t\t\t\texif.setThumbnail(std::move(thumbnail),\n+\t\t\t\t\t\t Exif::Compression::JPEG);\n\n> > \n> > \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> > }\n> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > index 2ff8fb78..4784496b 100644\n> > --- a/src/android/jpeg/exif.h\n> > +++ b/src/android/jpeg/exif.h\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > \n> > #include \n> > \n> > @@ -60,8 +61,7 @@ public:\n> > \n> > \tvoid setOrientation(int orientation);\n> > \tvoid setSize(const libcamera::Size &size);\n> > -\tvoid setThumbnail(libcamera::Span thumbnail,\n> > -\t\t\t Compression compression);\n> > +\tvoid setThumbnail(Compression compression);\n> > \tvoid setTimestamp(time_t timestamp, std::chrono::milliseconds msec);\n> > \n> > \tvoid setGPSDateTimestamp(time_t timestamp);\n> > @@ -78,6 +78,10 @@ public:\n> > \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> > \t[[nodiscard]] int generate();\n> > \n> > +\tstd::vector& getThumbnailRaw() {\n> > +\t\treturn thumbnail_raw_;\n> > +\t}\n> > +\n> > private:\n> > \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag);\n> > \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > @@ -106,4 +110,6 @@ private:\n> > \n> > \tunsigned char *exifData_;\n> > \tunsigned int size_;\n> > +\n> > +\tstd::vector thumbnail_raw_;\n> \n> CamelCase everywhere please ;-)\n\nAnd I'd call it thumbnailData_, or just thumbnail_.\n\n> Once reviewed by someone else, I 'll handle the merge so,\n> \n> Reviewed-by: Umang Jain \n> \n> > };\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index d72ebc3c..506b00b9 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -163,10 +163,8 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> > \t\tresultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, quality);\n> > \n> > \t\tif (thumbnailSize != Size(0, 0)) {\n> > -\t\t\tstd::vector thumbnail;\n> > -\t\t\tgenerateThumbnail(source, thumbnailSize, quality, &thumbnail);\n> > -\t\t\tif (!thumbnail.empty())\n> > -\t\t\t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n> > +\t\t\tgenerateThumbnail(source, thumbnailSize, quality, &exif.getThumbnailRaw());\n> > +\t\t\texif.setThumbnail(Exif::Compression::JPEG);\n> > \t\t}\n> > \n> > \t\tresultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 22F4CBE173\n\tfor ;\n\tThu, 21 Jul 2022 19:30:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43ED06330F;\n\tThu, 21 Jul 2022 21:30:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39FF0601B8\n\tfor ;\n\tThu, 21 Jul 2022 21:30:43 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 88544496;\n\tThu, 21 Jul 2022 21:30:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658431844;\n\tbh=XPt/ijjKgW6OC5I2sw3B/CRAlgEIqoHSGNTecdKQX8o=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=03GSzQbulMcZ8V6TUaiU75OCi6FbDcR8k1fufxVHFxZBiUEjKLBJQN4ZWxAIvg3Ij\n\t6qZw9PYS6FdL96mnKtnLypEDR4DRzxHS5xpksYc3R24ONZPYwETsOqH8ww7axq9C2+\n\ttpA/OQ86JFQxcddGz6PE+W840BAhz+sw2KL8dwPqSCFPB1HA1vMH27Fl9YVtR/frHl\n\t/bEczeLguyg0nXd/k0vk9kWhZj+/rLQfTIqMSwrxRYztM6KJr8p4aHLV8y4s3wG7+t\n\tAvtzD0Pao3YNiHYfCG5Ji61FB+e74A4zG1SvDB0dJ5nKp1WiF2ZPFYotEV+mZLkLzj\n\tw3bO3CqG0xHCw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658431842;\n\tbh=XPt/ijjKgW6OC5I2sw3B/CRAlgEIqoHSGNTecdKQX8o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Jn7k5M3mEpGh6kLW+tfdSywjunNEjM87Y+HOY9kfKR7NxA0gg5U4j09DGlVpN01Ko\n\tI08v9PRT/8QKPIY31TGEosA2EZx3VNb5f/NZ28amPeMYTBhaWwmcVM85+RIgZ4LROm\n\tw8llkHUSwDu2dKrMnDiiS+6DNkPWYDwMipc2xpZw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Jn7k5M3m\"; dkim-atps=neutral","Date":"Thu, 21 Jul 2022 22:30:40 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220720095812.2380218-1-chenghaoyang@google.com>\n\t<20220720095812.2380218-2-chenghaoyang@google.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail\n\tbuffer lifetime","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Harvey Yang ,\n\tHarvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24049,"web_url":"https://patchwork.libcamera.org/comment/24049/","msgid":"","date":"2022-07-22T07:07:15","subject":"Re: [libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail\n\tbuffer lifetime","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Umang & Laurent!\n\nFollowed most of your suggestions and sent the PATCH v2. Please check :)\n\nBR,\nHarvey\n\nOn Fri, Jul 22, 2022 at 3:30 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hello,\n>\n> On Thu, Jul 21, 2022 at 11:14:16PM +0530, Umang Jain via libcamera-devel\n> wrote:\n> > On 7/20/22 15:28, Harvey Yang via libcamera-devel wrote:\n> > > Previously the thumbnail buffer is destructed before even being used in\n> >\n> > Great catch, I think this was the reason for flaky failure for\n> > Camera3SimpleStillCaptureTest.JpegExifTest as mentioned!\n>\n> Oh indeed ! Nice !\n>\n> > > Exif. This patch moves the buffer into class Exif, so that the\n> developer\n> > > won't need to worry about its lifetime.\n> > >\n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > src/android/jpeg/exif.cpp | 10 ++++++----\n> > > src/android/jpeg/exif.h | 10 ++++++++--\n> > > src/android/jpeg/post_processor_jpeg.cpp | 6 ++----\n> > > 3 files changed, 16 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > > index 3220b458..584a29d0 100644\n> > > --- a/src/android/jpeg/exif.cpp\n> > > +++ b/src/android/jpeg/exif.cpp\n> > > @@ -435,11 +435,13 @@ void Exif::setOrientation(int orientation)\n> > > * Failing to do so, might result in no thumbnail data being set\n> even after a\n> > > * call to Exif::setThumbnail().\n> > > */\n>\n> Note that the issue is documented here. Clearly that wasn't enough, so\n> the API isn't good and needs to be changed. The comment can thus be\n> dropped.\n>\n> > > -void Exif::setThumbnail(Span thumbnail,\n> > > - Compression compression)\n> > > +void Exif::setThumbnail(Compression compression)\n> > > {\n> > > - data_->data = const_cast(thumbnail.data());\n> > > - data_->size = thumbnail.size();\n> > > + if (thumbnail_raw_.empty())\n> > > + return;\n> > > +\n> > > + data_->data = const_cast(thumbnail_raw_.data());\n> >\n> > I think we can do away with the const_cast here? Might not be compatible\n> > with previous C++ versions?\n> >\n> > I quickly compile tested\n> >\n> > - data_->data = const_cast(thumbnail_raw_.data());\n> > + data_->data = thumbnail_raw_.data();\n> >\n> > on clang version 12.0.1 and gcc (GCC) 11.0.1 which seems OK.\n>\n> There should be no need for a const_cast, indeed.\n>\n> > > + data_->size = thumbnail_raw_.size();\n>\n> I would do this a bit differently, to avoid the getThumbnailRaw(), which\n> really creates an interdependency between the Exif class and the\n> PostProcessorJpeg class.\n>\n> void Exif::setThumbnail(std::vector &&data, Compression\n> compression)\n> {\n> thumbnail_raw_ = std::move(data);\n>\n> data_->data = thumbnail_raw_.data();\n> data_->size = thumbnail_raw_.size();\n>\n> setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> }\n>\n> The only change to the caller compared to the current code would then be\n>\n> - exif.setThumbnail(thumbnail,\n> Exif::Compression::JPEG);\n> + exif.setThumbnail(std::move(thumbnail),\n> + Exif::Compression::JPEG);\n>\n> > >\n> > > setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> > > }\n> > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > > index 2ff8fb78..4784496b 100644\n> > > --- a/src/android/jpeg/exif.h\n> > > +++ b/src/android/jpeg/exif.h\n> > > @@ -10,6 +10,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -60,8 +61,7 @@ public:\n> > >\n> > > void setOrientation(int orientation);\n> > > void setSize(const libcamera::Size &size);\n> > > - void setThumbnail(libcamera::Span thumbnail,\n> > > - Compression compression);\n> > > + void setThumbnail(Compression compression);\n> > > void setTimestamp(time_t timestamp, std::chrono::milliseconds\n> msec);\n> > >\n> > > void setGPSDateTimestamp(time_t timestamp);\n> > > @@ -78,6 +78,10 @@ public:\n> > > libcamera::Span data() const { return { exifData_,\n> size_ }; }\n> > > [[nodiscard]] int generate();\n> > >\n> > > + std::vector& getThumbnailRaw() {\n> > > + return thumbnail_raw_;\n> > > + }\n> > > +\n> > > private:\n> > > ExifEntry *createEntry(ExifIfd ifd, ExifTag tag);\n> > > ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > > @@ -106,4 +110,6 @@ private:\n> > >\n> > > unsigned char *exifData_;\n> > > unsigned int size_;\n> > > +\n> > > + std::vector thumbnail_raw_;\n> >\n> > CamelCase everywhere please ;-)\n>\n> And I'd call it thumbnailData_, or just thumbnail_.\n>\n> > Once reviewed by someone else, I 'll handle the merge so,\n> >\n> > Reviewed-by: Umang Jain \n> >\n> > > };\n> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp\n> b/src/android/jpeg/post_processor_jpeg.cpp\n> > > index d72ebc3c..506b00b9 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > @@ -163,10 +163,8 @@ void\n> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu\n> > > resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,\n> quality);\n> > >\n> > > if (thumbnailSize != Size(0, 0)) {\n> > > - std::vector thumbnail;\n> > > - generateThumbnail(source, thumbnailSize, quality,\n> &thumbnail);\n> > > - if (!thumbnail.empty())\n> > > - exif.setThumbnail(thumbnail,\n> Exif::Compression::JPEG);\n> > > + generateThumbnail(source, thumbnailSize, quality,\n> &exif.getThumbnailRaw());\n> > > + exif.setThumbnail(Exif::Compression::JPEG);\n> > > }\n> > >\n> > > resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n> data, 2);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3866ABD1F1\n\tfor ;\n\tFri, 22 Jul 2022 07:07:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E07EB63311;\n\tFri, 22 Jul 2022 09:07:28 +0200 (CEST)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E699F6330B\n\tfor ;\n\tFri, 22 Jul 2022 09:07:26 +0200 (CEST)","by mail-lj1-x22c.google.com with SMTP id r14so4427725ljp.2\n\tfor ;\n\tFri, 22 Jul 2022 00:07:26 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658473648;\n\tbh=8ZR2SuTgDc4guKedTd/+PDSjR9pODMRIW1ZnuafzoMI=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yWp3zFyeJUNI0P3z4T4kodoaa3FJBp1JO0iiYtnaReM2w/roNhe+1MYoyb/YBraAd\n\tOOor8Wt627c2GARK7FowmcSqyDlgd4k/nKdl75my8QpLasDRDl6lwPg1q2MMT/HSWO\n\t4nKPCkp1+9MaEBI/X4Amw+Jx6+15bD0O80V+dfGQYg8z7nrX1lMlN+oihaqEI1ZlK0\n\tSDGBFTeNAqu25VmDA4OyFwuCG+UN5Qzja/Qew7InKR5e4rgpa0vNOJXtYeyz1DtbGH\n\tpHl2w8oWmDN37iltnYzlUWfYQpxMxyc5JxeMJ71vCejpl+r/+cJEbOvM4VTCZW/k3P\n\tGCZjV9sUJQBqg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oczZI/BvpJoCazUl2mXMUOTLOF1Jg2m+XKg1XFs24/0=;\n\tb=WVcP+8m21iAl/DmleQ7F4B/vHKLWXpeFqzcGBWHwXdKRt7bTly4jJJ6wusEz5zHV5m\n\thg929RmfFjmtdBu0/OZo+1PTfrARskwpj7hvaqeD5zQUR+U4vwid2eL32mkX21fSiv3A\n\t4wYoZ7TXuHEk9zigvwyD5oN7098faV3ikUpNM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"WVcP+8m2\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oczZI/BvpJoCazUl2mXMUOTLOF1Jg2m+XKg1XFs24/0=;\n\tb=aqv3SELQrVi0N8ce6E5d+KhADLdI2e/1zXAsxZJ4H14Mf029fwtU9iVxlvBCKc5EJR\n\tVaRTajJOUMsqK1wQlrrsFk7aIKPw3fPrU6RA1eoDmb2QxcMT7Xi/018AKOgzw7pXSuRN\n\tzuY4jyRhl+b/B4usOPvfCUfjLR2csr1WvagKdKPFa4cJSNkQDjt2NIvNVubih09IcWBR\n\tcX4fhA1P2qqofJSF+sMFHYJYaobXn8sqqWCQdxfm9QhWY7nxNKlh/cKa/8NLXaHKpNwo\n\tNv5m7v5WTCXe6W8bBw3tkmyAPGy1ubq0uN//JVLpjo1Nh7h3kNsHLwZJ4PD+Fj6SjZAe\n\tefzg==","X-Gm-Message-State":"AJIora+7lXradNd8wIJSZQRuQ84D4vJ6FZWVRBXdpoiMBHt8n1jE965Y\n\tJGLnqwVSV3J2n/WKLgEfg52V/TI8Uo5a52raoZUDYTJl9hM=","X-Google-Smtp-Source":"AGRyM1tdVygfkEGlXDx2Jdcj6Rnc1bIzj+54r0JO/lr223vrm9JUVBXV8mPu97h0+dZaJwPYh2zDEx/u06MXORu1HpM=","X-Received":"by 2002:a05:651c:220d:b0:25d:5fa0:a83a with SMTP id\n\ty13-20020a05651c220d00b0025d5fa0a83amr778371ljq.276.1658473646205;\n\tFri, 22 Jul 2022 00:07:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20220720095812.2380218-1-chenghaoyang@google.com>\n\t<20220720095812.2380218-2-chenghaoyang@google.com>\n\t\n\t","In-Reply-To":"","Date":"Fri, 22 Jul 2022 15:07:15 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000008d4ea905e45f7c20\"","Subject":"Re: [libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail\n\tbuffer lifetime","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]