[{"id":19640,"web_url":"https://patchwork.libcamera.org/comment/19640/","msgid":"<20210913140805.bedlinmwvpeg7np2@uno.localdomain>","date":"2021-09-13T14:08:05","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote:\n> Generation of thumbnail is not occuring currently because\n> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed\n> to PostProcessorJpeg::process(). This is a regression introduced in\n> 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request\n> metadata\").\n\nI see the issue, but I'm not sure those commit was wrong. I mean, what\nthat commit does that itroduced a regression is:\n\n--- a/src/android/jpeg/post_processor_jpeg.cpp\n+++ b/src/android/jpeg/post_processor_jpeg.cpp\n+ ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);\n+ if (ret) {\n+ const int32_t *data = entry.data.i32;\n+ Size thumbnailSize = { static_cast(data[0]),\n+ static_cast(data[1]) };\n+\n+ if (thumbnailSize != Size(0, 0)) {\n+ std::vector thumbnail;\n+ generateThumbnail(source, thumbnailSize, &thumbnail);\n+ if (!thumbnail.empty())\n+ exif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n+ }\n+\n+ resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);\n+\n+ /* \\todo Use this quality as a parameter to the encoder */\n+ ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);\n+ if (ret)\n+ resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,\n+ entry.data.u8, 1);\n+ }\n\n\nWhich I read as: \"If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in\nthe request's settings, do not populate it in result metadata\".\n\nIs this correct in your opinion, or should we populate it regardless\nof the fact the tag was passed in ?\n\n>\n> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in\n> the request metadata template populated by\n> CameraCapabilities::requestTemplatePreview(). The value for\n> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size\n> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n\nLet's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the\ncapture request template generated for the preview use case. I wonder\nif the JPEG thunbnail size should be part of the preview template now.\n\n>\n> Fixes: 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request metadata\")\n> Signed-off-by: Umang Jain \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_capabilities.cpp | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index e92bca42..76dddafd 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -1341,9 +1341,9 @@ std::unique_ptr CameraCapabilities::requestTemplatePreview() con\n> {\n> \t/*\n> \t * \\todo Keep this in sync with the actual number of entries.\n> -\t * Currently: 20 entries, 35 bytes\n> +\t * Currently: 21 entries, 37 bytes\n> \t */\n> -\tauto requestTemplate = std::make_unique(21, 36);\n> +\tauto requestTemplate = std::make_unique(22, 38);\n\nComment says 21, code says 22.\n\nIt was there already as the comment was not aligned with the code, but\nsince you're at it you could fix it\n\n> \tif (!requestTemplate->isValid()) {\n> \t\treturn nullptr;\n> \t}\n> @@ -1364,6 +1364,16 @@ std::unique_ptr CameraCapabilities::requestTemplatePreview() con\n> \trequestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,\n> \t\t\t\t entry.data.i32, 2);\n>\n> +\t/*\n> +\t * Get thumbnail sizes from static metadata and add the first non-zero\n> +\t * size to the template.\n> +\t */\n> +\tfound = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> +\t\t\t\t\t &entry);\n> +\tASSERT(found && entry.count >= 4);\n> +\trequestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n> +\t\t\t\t entry.data.i32 + 2, 2);\n> +\n\nIf I were to be extra paranoid I would make sure we actually discard\n(0, 0). The code assumes the first two entries are (0, 0) which is\nfine as we populate it after all.\n\nIf you don't want to\n\n unsigned int j = 0;\n while (j < entry.count / 2) {\n if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] == 0)\n continue;\n\n requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n entry.data.i32 + j, 2);\n }\n\nCould you at least capture that we assume the first two entries are\n(0,0) ?\n\nThanks\n j\n\n\n> \tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n> \trequestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n>\n> --\n> 2.31.1\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 5B2A8BDC71\n\tfor ;\n\tMon, 13 Sep 2021 14:07:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D790369182;\n\tMon, 13 Sep 2021 16:07:19 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DA966024C\n\tfor ;\n\tMon, 13 Sep 2021 16:07:18 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 2037EFF805;\n\tMon, 13 Sep 2021 14:07:17 +0000 (UTC)"],"Date":"Mon, 13 Sep 2021 16:08:05 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210913140805.bedlinmwvpeg7np2@uno.localdomain>","References":"<20210913040110.13789-1-umang.jain@ideasonboard.com>\n\t<20210913040110.13789-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210913040110.13789-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19641,"web_url":"https://patchwork.libcamera.org/comment/19641/","msgid":"<20210913141605.splqj4wexxrnpmvr@uno.localdomain>","date":"2021-09-13T14:16:05","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Sep 13, 2021 at 04:08:05PM +0200, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote:\n> > Generation of thumbnail is not occuring currently because\n> > ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed\n> > to PostProcessorJpeg::process(). This is a regression introduced in\n> > 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request\n> > metadata\").\n>\n> I see the issue, but I'm not sure those commit was wrong. I mean, what\n> that commit does that itroduced a regression is:\n>\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);\n> + if (ret) {\n> + const int32_t *data = entry.data.i32;\n> + Size thumbnailSize = { static_cast(data[0]),\n> + static_cast(data[1]) };\n> +\n> + if (thumbnailSize != Size(0, 0)) {\n> + std::vector thumbnail;\n> + generateThumbnail(source, thumbnailSize, &thumbnail);\n> + if (!thumbnail.empty())\n> + exif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n> + }\n> +\n> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);\n> +\n> + /* \\todo Use this quality as a parameter to the encoder */\n> + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);\n> + if (ret)\n> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,\n> + entry.data.u8, 1);\n> + }\n>\n>\n> Which I read as: \"If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in\n> the request's settings, do not populate it in result metadata\".\n>\n> Is this correct in your opinion, or should we populate it regardless\n> of the fact the tag was passed in ?\n>\n> >\n> > The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in\n> > the request metadata template populated by\n> > CameraCapabilities::requestTemplatePreview(). The value for\n> > ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size\n> > reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n>\n> Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the\n> capture request template generated for the preview use case. I wonder\n> if the JPEG thunbnail size should be part of the preview template now.\n>\n> >\n> > Fixes: 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request metadata\")\n> > Signed-off-by: Umang Jain \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > src/android/camera_capabilities.cpp | 14 ++++++++++++--\n> > 1 file changed, 12 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index e92bca42..76dddafd 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -1341,9 +1341,9 @@ std::unique_ptr CameraCapabilities::requestTemplatePreview() con\n> > {\n> > \t/*\n> > \t * \\todo Keep this in sync with the actual number of entries.\n> > -\t * Currently: 20 entries, 35 bytes\n> > +\t * Currently: 21 entries, 37 bytes\n> > \t */\n> > -\tauto requestTemplate = std::make_unique(21, 36);\n> > +\tauto requestTemplate = std::make_unique(22, 38);\n>\n> Comment says 21, code says 22.\n>\n> It was there already as the comment was not aligned with the code, but\n> since you're at it you could fix it\n>\n> > \tif (!requestTemplate->isValid()) {\n> > \t\treturn nullptr;\n> > \t}\n> > @@ -1364,6 +1364,16 @@ std::unique_ptr CameraCapabilities::requestTemplatePreview() con\n> > \trequestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,\n> > \t\t\t\t entry.data.i32, 2);\n> >\n> > +\t/*\n> > +\t * Get thumbnail sizes from static metadata and add the first non-zero\n> > +\t * size to the template.\n> > +\t */\n> > +\tfound = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> > +\t\t\t\t\t &entry);\n> > +\tASSERT(found && entry.count >= 4);\n> > +\trequestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n> > +\t\t\t\t entry.data.i32 + 2, 2);\n> > +\n>\n> If I were to be extra paranoid I would make sure we actually discard\n> (0, 0). The code assumes the first two entries are (0, 0) which is\n> fine as we populate it after all.\n>\n> If you don't want to\n>\n> unsigned int j = 0;\n> while (j < entry.count / 2) {\n> if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] == 0) {\n j += 2;\n\n :)\n\n> continue;\n }\n>\n> requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n> entry.data.i32 + j, 2);\n> }\n>\n> Could you at least capture that we assume the first two entries are\n> (0,0) ?\n>\n> Thanks\n> j\n>\n>\n> > \tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n> > \trequestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n> >\n> > --\n> > 2.31.1\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 50665BDB1D\n\tfor ;\n\tMon, 13 Sep 2021 14:15:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F17F69185;\n\tMon, 13 Sep 2021 16:15:19 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EDBBC6024C\n\tfor ;\n\tMon, 13 Sep 2021 16:15:18 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 68AF11BF20B;\n\tMon, 13 Sep 2021 14:15:18 +0000 (UTC)"],"Date":"Mon, 13 Sep 2021 16:16:05 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210913141605.splqj4wexxrnpmvr@uno.localdomain>","References":"<20210913040110.13789-1-umang.jain@ideasonboard.com>\n\t<20210913040110.13789-3-umang.jain@ideasonboard.com>\n\t<20210913140805.bedlinmwvpeg7np2@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210913140805.bedlinmwvpeg7np2@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19643,"web_url":"https://patchwork.libcamera.org/comment/19643/","msgid":"<86487fcd-9e27-3be2-ded3-720c0ef69b41@ideasonboard.com>","date":"2021-09-13T14:41:10","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 9/13/21 7:38 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote:\n>> Generation of thumbnail is not occuring currently because\n>> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed\n>> to PostProcessorJpeg::process(). This is a regression introduced in\n>> 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request\n>> metadata\").\n> I see the issue, but I'm not sure those commit was wrong. I mean, what\n> that commit does that itroduced a regression is:\n>\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);\n> + if (ret) {\n> + const int32_t *data = entry.data.i32;\n> + Size thumbnailSize = { static_cast(data[0]),\n> + static_cast(data[1]) };\n> +\n> + if (thumbnailSize != Size(0, 0)) {\n> + std::vector thumbnail;\n> + generateThumbnail(source, thumbnailSize, &thumbnail);\n> + if (!thumbnail.empty())\n> + exif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n> + }\n> +\n> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);\n> +\n> + /* \\todo Use this quality as a parameter to the encoder */\n> + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);\n> + if (ret)\n> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,\n> + entry.data.u8, 1);\n> + }\n>\n>\n> Which I read as: \"If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in\n> the request's settings, do not populate it in result metadata\".\n>\n> Is this correct in your opinion, or should we populate it regardless\n> of the fact the tag was passed in ?\n\n\nThis is correct, but the counterpart of the patch seems missing from \nthat commit, which actually resulted in regression\n\n>\n>> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in\n>> the request metadata template populated by\n>> CameraCapabilities::requestTemplatePreview(). The value for\n>> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size\n>> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n> Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the\n> capture request template generated for the preview use case. I wonder\n> if the JPEG thunbnail size should be part of the preview template now.\n\n\nI wonder that too...\n\n>\n>> Fixes: 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request metadata\")\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Laurent Pinchart \n>> ---\n>> src/android/camera_capabilities.cpp | 14 ++++++++++++--\n>> 1 file changed, 12 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n>> index e92bca42..76dddafd 100644\n>> --- a/src/android/camera_capabilities.cpp\n>> +++ b/src/android/camera_capabilities.cpp\n>> @@ -1341,9 +1341,9 @@ std::unique_ptr CameraCapabilities::requestTemplatePreview() con\n>> {\n>> \t/*\n>> \t * \\todo Keep this in sync with the actual number of entries.\n>> -\t * Currently: 20 entries, 35 bytes\n>> +\t * Currently: 21 entries, 37 bytes\n>> \t */\n>> -\tauto requestTemplate = std::make_unique(21, 36);\n>> +\tauto requestTemplate = std::make_unique(22, 38);\n> Comment says 21, code says 22.\n>\n> It was there already as the comment was not aligned with the code, but\n> since you're at it you could fix it\n\n\nLooking at both #entries and #bytes, that gives me an impression that \nextra entry and data buffer bytes are **intentional**, so I went ahead \nwith it! Are both of them a typo? I'll need some checking tomorrow.\n\n>\n>> \tif (!requestTemplate->isValid()) {\n>> \t\treturn nullptr;\n>> \t}\n>> @@ -1364,6 +1364,16 @@ std::unique_ptr CameraCapabilities::requestTemplatePreview() con\n>> \trequestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,\n>> \t\t\t\t entry.data.i32, 2);\n>>\n>> +\t/*\n>> +\t * Get thumbnail sizes from static metadata and add the first non-zero\n>> +\t * size to the template.\n>> +\t */\n>> +\tfound = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>> +\t\t\t\t\t &entry);\n>> +\tASSERT(found && entry.count >= 4);\n>> +\trequestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n>> +\t\t\t\t entry.data.i32 + 2, 2);\n>> +\n> If I were to be extra paranoid I would make sure we actually discard\n> (0, 0). The code assumes the first two entries are (0, 0) which is\n> fine as we populate it after all.\n\n\nNot only we populate it, it's a requirement that Size (0,0) should be in \nthe vector /and/ the vector needs to be sorted in ascending order [1] if \nmore sizes are provided with. We do the right thing when populating \nANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra paranoid here.\n\n\n[1]: \nhttps://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES\n\n>\n> If you don't want to\n>\n> unsigned int j = 0;\n> while (j < entry.count / 2) {\n> if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] == 0)\n> continue;\n>\n> requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n> entry.data.i32 + j, 2);\n> }\n>\n> Could you at least capture that we assume the first two entries are\n> (0,0) ?\n\n\nAs said above, it's a requirement for \nANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra clear \nagain here?\n\n>\n> Thanks\n> j\n>\n>\n>> \tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n>> \trequestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n>>\n>> --\n>> 2.31.1\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 30628BDB1D\n\tfor ;\n\tMon, 13 Sep 2021 14:41:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E88769184;\n\tMon, 13 Sep 2021 16:41:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E3F36024C\n\tfor ;\n\tMon, 13 Sep 2021 16:41:14 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.152])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AD324499;\n\tMon, 13 Sep 2021 16:41:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RlDyCUzJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631544074;\n\tbh=5Qsi2POqeO8ksxvJJRYttzKBuS/auiAh5tsSKUmQOYI=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=RlDyCUzJ/byFukSUD2542D1LkUI7iP+GFC7OIWaZKJFjZafx+ws0qpQ7cI7nRTN9Q\n\t2AfVioQW6vzZ/hQQplB2dcdLZFK7nNP3mmvDuqNhVHqEz6NjQPgUCNYO6kDMzDBUch\n\tDk9p7bCnleaEbM6Zsc0MYXZx/0h2992E09QYEeTg=","To":"Jacopo Mondi ","References":"<20210913040110.13789-1-umang.jain@ideasonboard.com>\n\t<20210913040110.13789-3-umang.jain@ideasonboard.com>\n\t<20210913140805.bedlinmwvpeg7np2@uno.localdomain>","From":"Umang Jain ","Message-ID":"<86487fcd-9e27-3be2-ded3-720c0ef69b41@ideasonboard.com>","Date":"Mon, 13 Sep 2021 20:11:10 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210913140805.bedlinmwvpeg7np2@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19736,"web_url":"https://patchwork.libcamera.org/comment/19736/","msgid":"<1ba0e8df-249f-602c-aadd-f085a495094a@ideasonboard.com>","date":"2021-09-21T11:50:01","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 9/13/21 8:11 PM, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 9/13/21 7:38 PM, Jacopo Mondi wrote:\n>> Hi Umang,\n>>\n>> On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote:\n>>> Generation of thumbnail is not occuring currently because\n>>> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed\n>>> to PostProcessorJpeg::process(). This is a regression introduced in\n>>> 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request\n>>> metadata\").\n>> I see the issue, but I'm not sure those commit was wrong. I mean, what\n>> that commit does that itroduced a regression is:\n>>\n>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>> +       ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, \n>> &entry);\n>> +       if (ret) {\n>> +               const int32_t *data = entry.data.i32;\n>> +               Size thumbnailSize = { static_cast(data[0]),\n>> + static_cast(data[1]) };\n>> +\n>> +               if (thumbnailSize != Size(0, 0)) {\n>> +                       std::vector thumbnail;\n>> +                       generateThumbnail(source, thumbnailSize, \n>> &thumbnail);\n>> +                       if (!thumbnail.empty())\n>> +                               exif.setThumbnail(thumbnail, \n>> Exif::Compression::JPEG);\n>> +               }\n>> +\n>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);\n>> +\n>> +               /* \\todo Use this quality as a parameter to the \n>> encoder */\n>> +               ret = \n>> requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);\n>> +               if (ret)\n>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,\n>> +                                                entry.data.u8, 1);\n>> +       }\n>>\n>>\n>> Which I read as: \"If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in\n>> the request's settings, do not populate it in result metadata\".\n>>\n>> Is this correct in your opinion, or should we populate it regardless\n>> of the fact the tag was passed in ?\n>\n>\n> This is correct, but the counterpart of the patch seems missing from \n> that commit, which actually resulted in regression\n>\n>>\n>>> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in\n>>> the request metadata template populated by\n>>> CameraCapabilities::requestTemplatePreview(). The value for\n>>> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size\n>>> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n>> Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the\n>> capture request template generated for the preview use case. I wonder\n>> if the JPEG thunbnail size should be part of the preview template now.\n>\n>\n> I wonder that too...\n>\n>>\n>>> Fixes: 1264628d3c92(\"android: jpeg: Configure thumbnailer based on \n>>> request metadata\")\n>>> Signed-off-by: Umang Jain \n>>> Reviewed-by: Laurent Pinchart \n>>> ---\n>>>   src/android/camera_capabilities.cpp | 14 ++++++++++++--\n>>>   1 file changed, 12 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_capabilities.cpp \n>>> b/src/android/camera_capabilities.cpp\n>>> index e92bca42..76dddafd 100644\n>>> --- a/src/android/camera_capabilities.cpp\n>>> +++ b/src/android/camera_capabilities.cpp\n>>> @@ -1341,9 +1341,9 @@ std::unique_ptr \n>>> CameraCapabilities::requestTemplatePreview() con\n>>>   {\n>>>       /*\n>>>        * \\todo Keep this in sync with the actual number of entries.\n>>> -     * Currently: 20 entries, 35 bytes\n>>> +     * Currently: 21 entries, 37 bytes\n>>>        */\n>>> -    auto requestTemplate = std::make_unique(21, 36);\n>>> +    auto requestTemplate = std::make_unique(22, 38);\n>> Comment says 21, code says 22.\n>>\n>> It was there already as the comment was not aligned with the code, but\n>> since you're at it you could fix it\n>\n>\n> Looking at both #entries and #bytes, that gives me an impression that \n> extra entry and data buffer bytes are **intentional**, so I went ahead \n> with it! Are both of them a typo? I'll need some checking tomorrow.\n\n\nIndeed, my assumption was a mistake. I think the mismatch was long \nsitting under the hood, and never really sync-ed with all the rework \naround CameraMetadata and request templates however,\n\nI found this ancient mistake where it was supposed to match, but \nunfortunately didn't, and the development/increment (as new entries were \nadded) kept happening on top of it.\n\nhttps://git.linuxtv.org/libcamera.git/commit/?id=637034742f2b0b7524\n\nI'll mention this as a point of divergence in v3 for this particular patch.\n\n>\n>>\n>>>       if (!requestTemplate->isValid()) {\n>>>           return nullptr;\n>>>       }\n>>> @@ -1364,6 +1364,16 @@ std::unique_ptr \n>>> CameraCapabilities::requestTemplatePreview() con\n>>> requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,\n>>>                     entry.data.i32, 2);\n>>>\n>>> +    /*\n>>> +     * Get thumbnail sizes from static metadata and add the first \n>>> non-zero\n>>> +     * size to the template.\n>>> +     */\n>>> +    found = \n>>> staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>>> +                      &entry);\n>>> +    ASSERT(found && entry.count >= 4);\n>>> +    requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n>>> +                  entry.data.i32 + 2, 2);\n>>> +\n>> If I were to be extra paranoid I would make sure we actually discard\n>> (0, 0). The code assumes the first two entries are (0, 0) which is\n>> fine as we populate it after all.\n>\n>\n> Not only we populate it, it's a requirement that Size (0,0) should be \n> in the vector /and/ the vector needs to be sorted in ascending order \n> [1] if more sizes are provided with. We do the right thing when \n> populating ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra \n> paranoid here.\n>\n>\n> [1]: \n> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES\n>\n>>\n>> If you don't want to\n>>\n>>          unsigned int j = 0;\n>>          while (j < entry.count / 2) {\n>>                  if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] \n>> == 0)\n>>                          continue;\n>>\n>> requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n>>                                            entry.data.i32 + j, 2);\n>>          }\n>>\n>> Could you at least capture that we assume the first two entries are\n>> (0,0) ?\n\n\nI am re-thinking this. It makes sense. It's one step more defensive than the\n\n     ASSERT(found && entry.count >= 4);\n\nIt's atleast for the better.\n\nI'll send a v3 for this patch (the 1/2 has already been merged) and also \ntest out the thumbnail generation with the v3 and we can put an end to \nthis thumbnail saga...\n\nThanks for your thoughts!\n\n\n>\n> As said above, it's a requirement for \n> ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra \n> clear again here?\n>\n>>\n>> Thanks\n>>     j\n>>\n>>\n>>>       uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n>>>       requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n>>>\n>>> -- \n>>> 2.31.1\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 C3370BF01C\n\tfor ;\n\tTue, 21 Sep 2021 11:50:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F0286918A;\n\tTue, 21 Sep 2021 13:50:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCDEF60247\n\tfor ;\n\tTue, 21 Sep 2021 13:50:07 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.144])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE2F12BA;\n\tTue, 21 Sep 2021 13:50:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hn2JzWUs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632225007;\n\tbh=8QXpYxho1n0C9UgQCtgDnYEJJYfUVsNcZCYuwHu4uJk=;\n\th=Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=hn2JzWUsaLcGQBgZ8eckmo0/nLmQCN9/DcDbCjQkudq2PVvthoScPEU41mV2TTy38\n\tpuKO/CeviPgYuSASsU06ZKqWi7SfaiN55SVIRvcgbly9HQwspDpf/zJ8WHOBSNZDWH\n\tAxb4O1PKmymVROzIkW80hyXgeTW6hEuVMDLEgntY=","From":"Umang Jain ","To":"Jacopo Mondi ","References":"<20210913040110.13789-1-umang.jain@ideasonboard.com>\n\t<20210913040110.13789-3-umang.jain@ideasonboard.com>\n\t<20210913140805.bedlinmwvpeg7np2@uno.localdomain>\n\t<86487fcd-9e27-3be2-ded3-720c0ef69b41@ideasonboard.com>","Message-ID":"<1ba0e8df-249f-602c-aadd-f085a495094a@ideasonboard.com>","Date":"Tue, 21 Sep 2021 17:20:01 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<86487fcd-9e27-3be2-ded3-720c0ef69b41@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19737,"web_url":"https://patchwork.libcamera.org/comment/19737/","msgid":"<476b96d5-d944-63c9-31d3-f81e2b39185d@ideasonboard.com>","date":"2021-09-21T11:52:10","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"On 9/21/21 5:20 PM, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 9/13/21 8:11 PM, Umang Jain wrote:\n>> Hi Jacopo\n>>\n>> On 9/13/21 7:38 PM, Jacopo Mondi wrote:\n>>> Hi Umang,\n>>>\n>>> On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote:\n>>>> Generation of thumbnail is not occuring currently because\n>>>> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed\n>>>> to PostProcessorJpeg::process(). This is a regression introduced in\n>>>> 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request\n>>>> metadata\").\n>>> I see the issue, but I'm not sure those commit was wrong. I mean, what\n>>> that commit does that itroduced a regression is:\n>>>\n>>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n>>> +       ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, \n>>> &entry);\n>>> +       if (ret) {\n>>> +               const int32_t *data = entry.data.i32;\n>>> +               Size thumbnailSize = { static_cast(data[0]),\n>>> + static_cast(data[1]) };\n>>> +\n>>> +               if (thumbnailSize != Size(0, 0)) {\n>>> +                       std::vector thumbnail;\n>>> +                       generateThumbnail(source, thumbnailSize, \n>>> &thumbnail);\n>>> +                       if (!thumbnail.empty())\n>>> +                               exif.setThumbnail(thumbnail, \n>>> Exif::Compression::JPEG);\n>>> +               }\n>>> +\n>>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);\n>>> +\n>>> +               /* \\todo Use this quality as a parameter to the \n>>> encoder */\n>>> +               ret = \n>>> requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);\n>>> +               if (ret)\n>>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,\n>>> + entry.data.u8, 1);\n>>> +       }\n>>>\n>>>\n>>> Which I read as: \"If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in\n>>> the request's settings, do not populate it in result metadata\".\n>>>\n>>> Is this correct in your opinion, or should we populate it regardless\n>>> of the fact the tag was passed in ?\n>>\n>>\n>> This is correct, but the counterpart of the patch seems missing from \n>> that commit, which actually resulted in regression\n>>\n>>>\n>>>> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in\n>>>> the request metadata template populated by\n>>>> CameraCapabilities::requestTemplatePreview(). The value for\n>>>> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size\n>>>> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n>>> Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the\n>>> capture request template generated for the preview use case. I wonder\n>>> if the JPEG thunbnail size should be part of the preview template now.\n>>\n>>\n>> I wonder that too...\n>>\n>>>\n>>>> Fixes: 1264628d3c92(\"android: jpeg: Configure thumbnailer based on \n>>>> request metadata\")\n>>>> Signed-off-by: Umang Jain \n>>>> Reviewed-by: Laurent Pinchart \n>>>> ---\n>>>>   src/android/camera_capabilities.cpp | 14 ++++++++++++--\n>>>>   1 file changed, 12 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_capabilities.cpp \n>>>> b/src/android/camera_capabilities.cpp\n>>>> index e92bca42..76dddafd 100644\n>>>> --- a/src/android/camera_capabilities.cpp\n>>>> +++ b/src/android/camera_capabilities.cpp\n>>>> @@ -1341,9 +1341,9 @@ std::unique_ptr \n>>>> CameraCapabilities::requestTemplatePreview() con\n>>>>   {\n>>>>       /*\n>>>>        * \\todo Keep this in sync with the actual number of entries.\n>>>> -     * Currently: 20 entries, 35 bytes\n>>>> +     * Currently: 21 entries, 37 bytes\n>>>>        */\n>>>> -    auto requestTemplate = std::make_unique(21, 36);\n>>>> +    auto requestTemplate = std::make_unique(22, 38);\n>>> Comment says 21, code says 22.\n>>>\n>>> It was there already as the comment was not aligned with the code, but\n>>> since you're at it you could fix it\n>>\n>>\n>> Looking at both #entries and #bytes, that gives me an impression that \n>> extra entry and data buffer bytes are **intentional**, so I went \n>> ahead with it! Are both of them a typo? I'll need some checking \n>> tomorrow.\n>\n>\n> Indeed, my assumption was a mistake. I think the mismatch was long \n> sitting under the hood, and never really sync-ed with all the rework \n> around CameraMetadata and request templates however,\n>\n> I found this ancient mistake where it was supposed to match, but \n> unfortunately didn't, and the development/increment (as new entries \n> were added) kept happening on top of it.\n\n\nerrr, I mean s/ancient mistake/ancient commit/\n\n>\n> https://git.linuxtv.org/libcamera.git/commit/?id=637034742f2b0b7524\n>\n> I'll mention this as a point of divergence in v3 for this particular \n> patch.\n>\n>>\n>>>\n>>>>       if (!requestTemplate->isValid()) {\n>>>>           return nullptr;\n>>>>       }\n>>>> @@ -1364,6 +1364,16 @@ std::unique_ptr \n>>>> CameraCapabilities::requestTemplatePreview() con\n>>>> requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,\n>>>>                     entry.data.i32, 2);\n>>>>\n>>>> +    /*\n>>>> +     * Get thumbnail sizes from static metadata and add the first \n>>>> non-zero\n>>>> +     * size to the template.\n>>>> +     */\n>>>> +    found = \n>>>> staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>>>> +                      &entry);\n>>>> +    ASSERT(found && entry.count >= 4);\n>>>> + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n>>>> +                  entry.data.i32 + 2, 2);\n>>>> +\n>>> If I were to be extra paranoid I would make sure we actually discard\n>>> (0, 0). The code assumes the first two entries are (0, 0) which is\n>>> fine as we populate it after all.\n>>\n>>\n>> Not only we populate it, it's a requirement that Size (0,0) should be \n>> in the vector /and/ the vector needs to be sorted in ascending order \n>> [1] if more sizes are provided with. We do the right thing when \n>> populating ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra \n>> paranoid here.\n>>\n>>\n>> [1]: \n>> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES\n>>\n>>>\n>>> If you don't want to\n>>>\n>>>          unsigned int j = 0;\n>>>          while (j < entry.count / 2) {\n>>>                  if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] \n>>> == 0)\n>>>                          continue;\n>>>\n>>> requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n>>>                                            entry.data.i32 + j, 2);\n>>>          }\n>>>\n>>> Could you at least capture that we assume the first two entries are\n>>> (0,0) ?\n>\n>\n> I am re-thinking this. It makes sense. It's one step more defensive \n> than the\n>\n>     ASSERT(found && entry.count >= 4);\n>\n> It's atleast for the better.\n>\n> I'll send a v3 for this patch (the 1/2 has already been merged) and \n> also test out the thumbnail generation with the v3 and we can put an \n> end to this thumbnail saga...\n>\n> Thanks for your thoughts!\n>\n>\n>>\n>> As said above, it's a requirement for \n>> ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra \n>> clear again here?\n>>\n>>>\n>>> Thanks\n>>>     j\n>>>\n>>>\n>>>>       uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n>>>>       requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n>>>>\n>>>> -- \n>>>> 2.31.1\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 8BFA6BF01C\n\tfor ;\n\tTue, 21 Sep 2021 11:52:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1639C6918A;\n\tTue, 21 Sep 2021 13:52:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10F7C60247\n\tfor ;\n\tTue, 21 Sep 2021 13:52:16 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.144])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC8C62BA;\n\tTue, 21 Sep 2021 13:52:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"X4ymaQmg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632225135;\n\tbh=2XyG+1957inaRBU0CYCnPnY/MtbhvTNYZKsJiaYN0uE=;\n\th=Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=X4ymaQmgmEJUPbk8jXC83Qa7otOwuBdlj3c8wcz1n9qAgGRGKDUomzLmjgNOTKhrd\n\tFwW+3WBqpjQ4pLhmtA+QI6Hg1MNT0DeKlhsQzjRhApmjPgBmmyIMll6mQL+ya7AKAw\n\tavUDSQzAadoBuvz6k/kQUeSaqJXTrwO3xLJohnm0=","From":"Umang Jain ","To":"Jacopo Mondi ","References":"<20210913040110.13789-1-umang.jain@ideasonboard.com>\n\t<20210913040110.13789-3-umang.jain@ideasonboard.com>\n\t<20210913140805.bedlinmwvpeg7np2@uno.localdomain>\n\t<86487fcd-9e27-3be2-ded3-720c0ef69b41@ideasonboard.com>\n\t<1ba0e8df-249f-602c-aadd-f085a495094a@ideasonboard.com>","Message-ID":"<476b96d5-d944-63c9-31d3-f81e2b39185d@ideasonboard.com>","Date":"Tue, 21 Sep 2021 17:22:10 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<1ba0e8df-249f-602c-aadd-f085a495094a@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19771,"web_url":"https://patchwork.libcamera.org/comment/19771/","msgid":"","date":"2021-09-22T06:19:05","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Tue, Sep 21, 2021 at 8:52 PM Umang Jain wrote:\n>\n>\n> On 9/21/21 5:20 PM, Umang Jain wrote:\n> > Hi Jacopo\n> >\n> > On 9/13/21 8:11 PM, Umang Jain wrote:\n> >> Hi Jacopo\n> >>\n> >> On 9/13/21 7:38 PM, Jacopo Mondi wrote:\n> >>> Hi Umang,\n> >>>\n> >>> On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote:\n> >>>> Generation of thumbnail is not occuring currently because\n> >>>> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed\n> >>>> to PostProcessorJpeg::process(). This is a regression introduced in\n> >>>> 1264628d3c92(\"android: jpeg: Configure thumbnailer based on request\n> >>>> metadata\").\n> >>> I see the issue, but I'm not sure those commit was wrong. I mean, what\n> >>> that commit does that itroduced a regression is:\n> >>>\n> >>> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> >>> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> >>> + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n> >>> &entry);\n> >>> + if (ret) {\n> >>> + const int32_t *data = entry.data.i32;\n> >>> + Size thumbnailSize = { static_cast(data[0]),\n> >>> + static_cast(data[1]) };\n> >>> +\n> >>> + if (thumbnailSize != Size(0, 0)) {\n> >>> + std::vector thumbnail;\n> >>> + generateThumbnail(source, thumbnailSize,\n> >>> &thumbnail);\n> >>> + if (!thumbnail.empty())\n> >>> + exif.setThumbnail(thumbnail,\n> >>> Exif::Compression::JPEG);\n> >>> + }\n> >>> +\n> >>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);\n> >>> +\n> >>> + /* \\todo Use this quality as a parameter to the\n> >>> encoder */\n> >>> + ret =\n> >>> requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);\n> >>> + if (ret)\n> >>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,\n> >>> + entry.data.u8, 1);\n> >>> + }\n> >>>\n> >>>\n> >>> Which I read as: \"If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in\n> >>> the request's settings, do not populate it in result metadata\".\n> >>>\n> >>> Is this correct in your opinion, or should we populate it regardless\n> >>> of the fact the tag was passed in ?\n> >>\n> >>\n> >> This is correct, but the counterpart of the patch seems missing from\n> >> that commit, which actually resulted in regression\n> >>\n> >>>\n> >>>> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in\n> >>>> the request metadata template populated by\n> >>>> CameraCapabilities::requestTemplatePreview(). The value for\n> >>>> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size\n> >>>> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n> >>> Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the\n> >>> capture request template generated for the preview use case. I wonder\n> >>> if the JPEG thunbnail size should be part of the preview template now.\n> >>\n> >>\n> >> I wonder that too...\n> >>\n> >>>\n> >>>> Fixes: 1264628d3c92(\"android: jpeg: Configure thumbnailer based on\n> >>>> request metadata\")\n> >>>> Signed-off-by: Umang Jain \n> >>>> Reviewed-by: Laurent Pinchart \n\nReviewed-by: Hirokazu Honda \n\n> >>>> ---\n> >>>> src/android/camera_capabilities.cpp | 14 ++++++++++++--\n> >>>> 1 file changed, 12 insertions(+), 2 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_capabilities.cpp\n> >>>> b/src/android/camera_capabilities.cpp\n> >>>> index e92bca42..76dddafd 100644\n> >>>> --- a/src/android/camera_capabilities.cpp\n> >>>> +++ b/src/android/camera_capabilities.cpp\n> >>>> @@ -1341,9 +1341,9 @@ std::unique_ptr\n> >>>> CameraCapabilities::requestTemplatePreview() con\n> >>>> {\n> >>>> /*\n> >>>> * \\todo Keep this in sync with the actual number of entries.\n> >>>> - * Currently: 20 entries, 35 bytes\n> >>>> + * Currently: 21 entries, 37 bytes\n> >>>> */\n> >>>> - auto requestTemplate = std::make_unique(21, 36);\n> >>>> + auto requestTemplate = std::make_unique(22, 38);\n> >>> Comment says 21, code says 22.\n> >>>\n> >>> It was there already as the comment was not aligned with the code, but\n> >>> since you're at it you could fix it\n> >>\n> >>\n> >> Looking at both #entries and #bytes, that gives me an impression that\n> >> extra entry and data buffer bytes are **intentional**, so I went\n> >> ahead with it! Are both of them a typo? I'll need some checking\n> >> tomorrow.\n> >\n> >\n> > Indeed, my assumption was a mistake. I think the mismatch was long\n> > sitting under the hood, and never really sync-ed with all the rework\n> > around CameraMetadata and request templates however,\n> >\n> > I found this ancient mistake where it was supposed to match, but\n> > unfortunately didn't, and the development/increment (as new entries\n> > were added) kept happening on top of it.\n>\n>\n> errr, I mean s/ancient mistake/ancient commit/\n>\n> >\n> > https://git.linuxtv.org/libcamera.git/commit/?id=637034742f2b0b7524\n> >\n> > I'll mention this as a point of divergence in v3 for this particular\n> > patch.\n> >\n> >>\n> >>>\n> >>>> if (!requestTemplate->isValid()) {\n> >>>> return nullptr;\n> >>>> }\n> >>>> @@ -1364,6 +1364,16 @@ std::unique_ptr\n> >>>> CameraCapabilities::requestTemplatePreview() con\n> >>>> requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,\n> >>>> entry.data.i32, 2);\n> >>>>\n> >>>> + /*\n> >>>> + * Get thumbnail sizes from static metadata and add the first\n> >>>> non-zero\n> >>>> + * size to the template.\n> >>>> + */\n> >>>> + found =\n> >>>> staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n> >>>> + &entry);\n> >>>> + ASSERT(found && entry.count >= 4);\n> >>>> + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n> >>>> + entry.data.i32 + 2, 2);\n> >>>> +\n> >>> If I were to be extra paranoid I would make sure we actually discard\n> >>> (0, 0). The code assumes the first two entries are (0, 0) which is\n> >>> fine as we populate it after all.\n> >>\n> >>\n> >> Not only we populate it, it's a requirement that Size (0,0) should be\n> >> in the vector /and/ the vector needs to be sorted in ascending order\n> >> [1] if more sizes are provided with. We do the right thing when\n> >> populating ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra\n> >> paranoid here.\n> >>\n> >>\n> >> [1]:\n> >> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES\n> >>\n> >>>\n> >>> If you don't want to\n> >>>\n> >>> unsigned int j = 0;\n> >>> while (j < entry.count / 2) {\n> >>> if (entry.data.i32[j] == 0 || entry.data.i32[j + 1]\n> >>> == 0)\n> >>> continue;\n> >>>\n> >>> requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,\n> >>> entry.data.i32 + j, 2);\n> >>> }\n> >>>\n> >>> Could you at least capture that we assume the first two entries are\n> >>> (0,0) ?\n> >\n> >\n> > I am re-thinking this. It makes sense. It's one step more defensive\n> > than the\n> >\n> > ASSERT(found && entry.count >= 4);\n> >\n> > It's atleast for the better.\n> >\n> > I'll send a v3 for this patch (the 1/2 has already been merged) and\n> > also test out the thumbnail generation with the v3 and we can put an\n> > end to this thumbnail saga...\n> >\n> > Thanks for your thoughts!\n> >\n> >\n> >>\n> >> As said above, it's a requirement for\n> >> ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra\n> >> clear again here?\n> >>\n> >>>\n> >>> Thanks\n> >>> j\n> >>>\n> >>>\n> >>>> uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;\n> >>>> requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n> >>>>\n> >>>> --\n> >>>> 2.31.1\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 D508DBF01C\n\tfor ;\n\tWed, 22 Sep 2021 06:19:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 346386918E;\n\tWed, 22 Sep 2021 08:19:17 +0200 (CEST)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5098D6012C\n\tfor ;\n\tWed, 22 Sep 2021 08:19:16 +0200 (CEST)","by mail-ed1-x530.google.com with SMTP id v22so5539010edd.11\n\tfor ;\n\tTue, 21 Sep 2021 23:19:16 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"AvEI7k1C\"; dkim-atps=neutral","DKIM-Signature":"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=uUTLl2DoMT7/KRf/YHpyjawFH92GgKBMUUXaJuhFXd8=;\n\tb=AvEI7k1CwfUwF3mHQNy+z85sXW/YflaPDkgUSmQcpxp85X8skHs00+BBRqGZDwOm9L\n\t1b3I4QaJgHWx6/k8ZuFCwCepOI3VW1tGaS5XZqp/btX2vknaXrq39I4HGFi6f38uhN8U\n\tK4TM8lpIeUsUdYFFNv+zmarGUeusaRtzAmQok=","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=uUTLl2DoMT7/KRf/YHpyjawFH92GgKBMUUXaJuhFXd8=;\n\tb=0il4AbNZpiRsiJOJKdZiGpv9i0E+2HXD3dlKmP2W/MWjLFZvU4QuzfgEDkNFEe7DmC\n\txn9uqwU165GSoqpJ/4aG4/dQxFQZFRdK2q79GvhcGa0oqSGEpe2+Qzu2NceGKposolbt\n\tkPRQPC2jVT9SkXQwxwXCnO0f2LrehNPlxeulFt8C6FZ+uqQTokcUNGWIFs1cXAkJI2Eh\n\tczQ179U4NlIBGjcAsO2TQJo6Jaj9QkVJuAnacKeuyCtlhcylXXsks5CMHCgND2yjW8c8\n\tUQdN0aZUcvtkBQgy6UdACxbqXhmtKjssZOJbQIR9FmwOkmcMDmt/0plxErEXbfgz18JX\n\tEYuQ==","X-Gm-Message-State":"AOAM531QrGX+kWaJHa3WxigizW4VTrcAf6nwZcWdtOhDWZIgFuC27Vj9\n\tZn4ZdKghtPQouGIZevidvNQiJaZgbhr/0nld6TIXL/WS6lU=","X-Google-Smtp-Source":"ABdhPJwyNcKT1SbKXh6VR85DO8DkYaINNIgVNHfqgAVhAXU65/1g2xUql9C7+KW+vZty+80izEh+YcBDSvkJrUF+ugc=","X-Received":"by 2002:a17:906:4fd6:: with SMTP id\n\ti22mr38833187ejw.92.1632291555843; \n\tTue, 21 Sep 2021 23:19:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210913040110.13789-1-umang.jain@ideasonboard.com>\n\t<20210913040110.13789-3-umang.jain@ideasonboard.com>\n\t<20210913140805.bedlinmwvpeg7np2@uno.localdomain>\n\t<86487fcd-9e27-3be2-ded3-720c0ef69b41@ideasonboard.com>\n\t<1ba0e8df-249f-602c-aadd-f085a495094a@ideasonboard.com>\n\t<476b96d5-d944-63c9-31d3-f81e2b39185d@ideasonboard.com>","In-Reply-To":"<476b96d5-d944-63c9-31d3-f81e2b39185d@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Wed, 22 Sep 2021 15:19:05 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Fix generation of\n\tthumbnail for EXIF data","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]