[{"id":21225,"web_url":"https://patchwork.libcamera.org/comment/21225/","msgid":"<163783831366.3059017.680104591520353446@Monstersaurus>","date":"2021-11-25T11:05:13","subject":"Re: [libcamera-devel] [PATCH 3/7] android: camera_metadata: Add\n\tappendEntry helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2021-11-23 10:40:38)\n> Add appendEntry() helper, that automatically detects if updateEntry() or\n> addEntry() should be used.\n> \n> For now only implement it for enums and arithmetic values, as they will\n> mainly be used in populating templates, where the preview template\n> generator may or may not have (based on capabilities) already added a\n> key.\n\nI'm curious, should this always be the case? or is it better to be\nexplicit to only use update when we know it could already be there to\nensure efficiencies.\n\nOtherwise it might be simpler to add this check into addEntry()\ndirectly?\n\nI assume we never want a tag to be added multiple times?\n\nIf this is explicitly separated for efficiency reasons then:\n\n\nReviewed-by: Kieran Bingham \n\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/android/camera_metadata.h | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n> \n> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> index 8555c7c3..cca14d6c 100644\n> --- a/src/android/camera_metadata.h\n> +++ b/src/android/camera_metadata.h\n> @@ -33,6 +33,17 @@ public:\n> \n> bool hasEntry(uint32_t tag) const;\n> \n> + template + std::enable_if_t ||\n> + std::is_enum_v> * = nullptr>\n> + bool appendEntry(uint32_t tag, const T &data)\n> + {\n> + if (hasEntry(tag))\n> + return updateEntry(tag, &data, 1, sizeof(T));\n> + else\n> + return addEntry(tag, &data, 1, sizeof(T));\n> + }\n> +\n> template std::enable_if_t ||\n> std::is_enum_v> * = nullptr>\n> -- \n> 2.27.0\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 05999BDB13\n\tfor ;\n\tThu, 25 Nov 2021 11:05:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F5936036F;\n\tThu, 25 Nov 2021 12:05:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F6E160231\n\tfor ;\n\tThu, 25 Nov 2021 12:05:16 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1846990E;\n\tThu, 25 Nov 2021 12:05:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KLBb3CdX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637838316;\n\tbh=x6UlSUSXq9v+HorP+bbNK2Vzl9fFNuOJcNkUfvPpdRU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=KLBb3CdXIuwI82pT0y++5dEdS1Pc5VWy17ok1CvdGUAeW9+McMWwW3NpjSjRTYR2y\n\tkx/vB1N0HkcEV8NmVNp1xkU76FCzMyXkoQsGYUcdTnOBQwt3n5IsF0lf3uHbVqmGwy\n\tLTtI+nCffId+HcORYQioBAG7Mar3oi9LQsItX6kQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211123104042.3100902-4-paul.elder@ideasonboard.com>","References":"<20211123104042.3100902-1-paul.elder@ideasonboard.com>\n\t<20211123104042.3100902-4-paul.elder@ideasonboard.com>","From":"Kieran Bingham ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 25 Nov 2021 11:05:13 +0000","Message-ID":"<163783831366.3059017.680104591520353446@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 3/7] android: camera_metadata: Add\n\tappendEntry helper","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21832,"web_url":"https://patchwork.libcamera.org/comment/21832/","msgid":"<20211220224805.GC2742@pyrite.rasen.tech>","date":"2021-12-20T22:48:05","subject":"Re: [libcamera-devel] [PATCH 3/7] android: camera_metadata: Add\n\tappendEntry helper","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Nov 25, 2021 at 11:05:13AM +0000, Kieran Bingham wrote:\n> Quoting Paul Elder (2021-11-23 10:40:38)\n> > Add appendEntry() helper, that automatically detects if updateEntry() or\n> > addEntry() should be used.\n> > \n> > For now only implement it for enums and arithmetic values, as they will\n> > mainly be used in populating templates, where the preview template\n> > generator may or may not have (based on capabilities) already added a\n> > key.\n> \n> I'm curious, should this always be the case? or is it better to be\n> explicit to only use update when we know it could already be there to\n> ensure efficiencies.\n\nupdateEntry() will fail if the entry is not yet added, and addEntry()\nwill fail if the entry is already added. It's not a nice noisy failure\neither, you get weird behavior and scratch your head like \"wtf I added\nthis key with this value what are you talking about\".\n\n> \n> Otherwise it might be simpler to add this check into addEntry()\n> directly?\n\nI considered it tbh. But decided against it for efficiency reasons (that\nwere never really quantitatively assesed).\n\n> \n> I assume we never want a tag to be added multiple times?\n\nYeah, it'll fail.\n\n> \n> If this is explicitly separated for efficiency reasons then:\n\nIt's for correctness. For keys up to now, the preview template generator\nwould *always* add a key, and the other template generators, that use\nthe preview template as a base, could reliably use updateEntry. Now we\ncan't anymore, since the preview template generator would do if\n(capability) { addEntry() }, so the later template generators can't\nguarantee updateEntry() is the correct function.\n\nI thought I explained this in the commit message :/\n\nClearly it wasn't sufficient. I'll add a line about correctness with\naddEntry and updateEntry.\n\n> \n> \n> Reviewed-by: Kieran Bingham \n\n\nThanks,\n\nPaul\n\n> \n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/android/camera_metadata.h | 11 +++++++++++\n> > 1 file changed, 11 insertions(+)\n> > \n> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > index 8555c7c3..cca14d6c 100644\n> > --- a/src/android/camera_metadata.h\n> > +++ b/src/android/camera_metadata.h\n> > @@ -33,6 +33,17 @@ public:\n> > \n> > bool hasEntry(uint32_t tag) const;\n> > \n> > + template > + std::enable_if_t ||\n> > + std::is_enum_v> * = nullptr>\n> > + bool appendEntry(uint32_t tag, const T &data)\n> > + {\n> > + if (hasEntry(tag))\n> > + return updateEntry(tag, &data, 1, sizeof(T));\n> > + else\n> > + return addEntry(tag, &data, 1, sizeof(T));\n> > + }\n> > +\n> > template > std::enable_if_t ||\n> > std::is_enum_v> * = nullptr>\n> > -- \n> > 2.27.0\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 1920FBE080\n\tfor ;\n\tMon, 20 Dec 2021 22:48:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F6E0608E8;\n\tMon, 20 Dec 2021 23:48:13 +0100 (CET)","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 E34A960868\n\tfor ;\n\tMon, 20 Dec 2021 23:48:11 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2604:2d80:ad90:fb00:96fd:8874:873:6c16])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0184CB9C;\n\tMon, 20 Dec 2021 23:48:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CysbsBhK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1640040491;\n\tbh=a7OALDHe/Zwlp2Utg4FHA2XYwey+E8iwqI+3fLb5i9c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CysbsBhKnG7oWJID6v+52g8rKfFvlmSRZ/aVNwzrIIhWRAunNMPw7CmwJfeBiv7o7\n\tboCH/+X/cfcqvk0RqsorwKDBreqQepWsQqLLNXqqD0vZqk+Lq4HTMjmhX6bMLyTmgB\n\tPJYLW6lA3ljRfGpDYnMUOU9E0f2NAV8hAKJvYQE0=","Date":"Mon, 20 Dec 2021 16:48:05 -0600","From":"paul.elder@ideasonboard.com","To":"Kieran Bingham ","Message-ID":"<20211220224805.GC2742@pyrite.rasen.tech>","References":"<20211123104042.3100902-1-paul.elder@ideasonboard.com>\n\t<20211123104042.3100902-4-paul.elder@ideasonboard.com>\n\t<163783831366.3059017.680104591520353446@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<163783831366.3059017.680104591520353446@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 3/7] android: camera_metadata: Add\n\tappendEntry helper","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\" "}}]