[{"id":16465,"web_url":"https://patchwork.libcamera.org/comment/16465/","msgid":"","date":"2021-04-21T18:40:37","subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote:\n> When a Request is completed upon receiving the IPA produced metadata,\n> the metadata associated with the Request are over-written, deleting\n> the information set at output buffer completion, such as the\n> SensorTimestamp.\n> \n> This commit applies to the RkISP1 pipeline handler the same change\n> applied to IPU3 in commit 13a7ed7b1f1f (\"libcamera: ipu3: Do not\n> over-write metadata\") but compared to that commit it uses the newly\n> introduced ControlList::merge() function.\n> \n> Fixes: 0eb65e14e18d (\"libcamera: pipeline: rkisp1: Attach to an IPA\")\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 549f4a4e61a8..c3d390f775f2 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> \tif (!info)\n> \t\treturn;\n> \n> -\tinfo->request->metadata() = metadata;\n> +\tinfo->request->metadata().merge(const_cast(metadata));\n\nI like the change but this cast makes me think twice of the merge() \nsignature. Could we make the argument a const reference?\n\n> \tinfo->metadataProcessed = true;\n> \n> \tpipe->tryCompleteRequest(info->request);\n> -- \n> 2.31.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 98C1FBDB17\n\tfor ;\n\tWed, 21 Apr 2021 18:40:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 208C86884C;\n\tWed, 21 Apr 2021 20:40:40 +0200 (CEST)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7213568840\n\tfor ;\n\tWed, 21 Apr 2021 20:40:39 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id u4so48853333ljo.6\n\tfor ;\n\tWed, 21 Apr 2021 11:40:39 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id v1sm34899ljj.77.2021.04.21.11.40.38\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 21 Apr 2021 11:40:38 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"U0C2KJ2M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=Zm1coyHHrHyCN/WWVsIAioIg+VDihyuaPGFATCs0nuo=;\n\tb=U0C2KJ2MWSgeR7TmW85csMHpw8oLmtugIAsLRmuNv01NL1t+PJJJ+R4s6TBfMdKTlO\n\tRuTCmwo5DboS3njNY5DhS15nWz63cHe8J97yFqugRDs+sQjMqXx71772rugt/Hw4VjyQ\n\tJFy/iCvdNP0u9JWuV67RVeydbkXxWmybwaWoxE106VSXMd7mEEMwVrr7rGFq289xtG1k\n\tmmHicia1hLN7BLNkr9YXlFd1OalLvGCHexAJ2ZLcmK12wWhqiOB+SrrNE8732stagjnh\n\th1yrmao9LFGUlsMrqH1R+TmlfuHyoOILddLqlsGVOj0TrAHhTN4mhcp9+LiZp8dnYs4B\n\tjnMA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=Zm1coyHHrHyCN/WWVsIAioIg+VDihyuaPGFATCs0nuo=;\n\tb=WbZ1VvdQuGSu0qBBjW6pjOmJ0wbTxRcRy9wDwbBF0CKgvu0bn9snwdtF6em1o1oSMV\n\t2f5y+ekY2egceDVLDZWVglVAMZo9b3oMHat1BzQnhgvRmIlU29rfSaW9uoQO/H58KrIr\n\tTXZjMuXtP2mhh0ZOmzhZUtlRkmxNqOlCbPyKhdspW1rQwR7n1aarculHmIjBDjHIbwkM\n\tICPCwWNmQQM5d+Es1pbs2P/N9VdkgnSbUTNCen3CY5L5k68U2iOKRzqcHRot+7ECYUob\n\tEkBC3yK2EYgV/VziRJI1kxocFl5E9wmbmjwJIBJ/PTy9zRApc0Qygdx6AmgmqLEoiXzN\n\tSiYA==","X-Gm-Message-State":"AOAM53236Vq7HMG1Yrisv2udOxYwBzKgF5B+d5x6ru3irfiR09mBhlSM\n\ttm8Z4ueuPPPMGw7Yko2RBw4S1g==","X-Google-Smtp-Source":"ABdhPJzPJi5mHuTomPs5/7c18Lw18TMCjXpmzw5bdLa/azv5/ETfjzL8WnNutMfLBy4akQhMGTM3Gg==","X-Received":"by 2002:a2e:711b:: with SMTP id\n\tm27mr19467207ljc.485.1619030438876; \n\tWed, 21 Apr 2021 11:40:38 -0700 (PDT)","Date":"Wed, 21 Apr 2021 20:40:37 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421160319.42251-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16471,"web_url":"https://patchwork.libcamera.org/comment/16471/","msgid":"","date":"2021-04-21T22:03:42","subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi again Jacopo,\n\nOn 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote:\n> When a Request is completed upon receiving the IPA produced metadata,\n> the metadata associated with the Request are over-written, deleting\n> the information set at output buffer completion, such as the\n> SensorTimestamp.\n> \n> This commit applies to the RkISP1 pipeline handler the same change\n> applied to IPU3 in commit 13a7ed7b1f1f (\"libcamera: ipu3: Do not\n> over-write metadata\") but compared to that commit it uses the newly\n> introduced ControlList::merge() function.\n> \n> Fixes: 0eb65e14e18d (\"libcamera: pipeline: rkisp1: Attach to an IPA\")\n\nThis fixes tag is incorrect. Testing on master the only metadata \nrecorded in the request is the AeLock. Applying this series the only \nmetadata reported is the AeLock and SensorTimestamp, and the timestamp \nis added by the next patch in this series. There is no regression here \nto fix, only preparation for additional work, or am I missing something?\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 549f4a4e61a8..c3d390f775f2 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> \tif (!info)\n> \t\treturn;\n> \n> -\tinfo->request->metadata() = metadata;\n> +\tinfo->request->metadata().merge(const_cast(metadata));\n> \tinfo->metadataProcessed = true;\n> \n> \tpipe->tryCompleteRequest(info->request);\n> -- \n> 2.31.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 7352BBDB15\n\tfor ;\n\tWed, 21 Apr 2021 22:03:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B370A68849;\n\tThu, 22 Apr 2021 00:03:47 +0200 (CEST)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B54C360516\n\tfor ;\n\tThu, 22 Apr 2021 00:03:45 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id u4so49464403ljo.6\n\tfor ;\n\tWed, 21 Apr 2021 15:03:45 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ty22sm75709lfl.196.2021.04.21.15.03.43\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 21 Apr 2021 15:03:43 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"k0p7bA8L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=YHJNWRrTwiHNr+pgVTUMLlZ+P6S7iS5rDPvgizweYRU=;\n\tb=k0p7bA8L/vvOR+nNuxNS94tiHbAJ6CLfJ/2q+5zXLdmouORiiQ5NRP+8LjYKDgwqtD\n\td5W6fXm4rVnQIXasGyUj78cua94Bz7PzoLT9tb9aZ1Gj627/77LtoA338m1h062czvGi\n\tZCr3iH8BwnbbaUpME6MtjJlrm6WidCZ+exib9vr5/X2LZ4n3VRyDVBq8AuntNK3c/6XP\n\trMkA7Z4fySAeNXtCj9GnW9xc2qaQlqpShpGMwy63zR48xA5od+gyObCXP6j8dLJpBZLx\n\toNfn23/8Q0k+A0GRapxo3icdvrs3mCMAyJZ4fJc+lRATyh9tD6IwMDsPtLZEYro351ZT\n\tFrUA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=YHJNWRrTwiHNr+pgVTUMLlZ+P6S7iS5rDPvgizweYRU=;\n\tb=NBgM3WP4BfvCqz7K/5t4/O5In3sW0X2L3V2NatVOJCVjs4iT2G53mzJgJxx3USWgxB\n\tGAsdCYOxpd/RUHBMd4ARqYJky2sIxiwWR9+FWYztl4ruyeaw6+0m4KWjAKRQgOnOkiQw\n\tACPSbx10DF7csGnHLonlJdwvXryhsHXLOsRFA+vo9xaNEWytzbuPvqsBDVroDX91sZ8H\n\tyBZzOS7D3fe0FH+D8VVOKk9SJ7Won4NedKcz86KT8tawdCS570m/BCYxhM4d5rqtFJib\n\to48QzxMcRgirIMP9EMEooH6Kbggs5+EeUHdgXSy1F7NcqIQyyjkjaXWNlHTBd4OZ+w9j\n\tXe/A==","X-Gm-Message-State":"AOAM531K1/6/UNdZk+QmQvv0CQ9TmgAMkC8dnrRl5jhcyGoHB5EbD4No\n\tmWndvh6lZULgbWZv8xtBzhVSTQ==","X-Google-Smtp-Source":"ABdhPJxLvCcKz9yoeFvcRt2FxWRDF9HX/vRY9tGOKzeCxjZcv2velGHlyr3cyt37YXKV9T60EH/kUQ==","X-Received":"by 2002:a2e:2e10:: with SMTP id u16mr268281lju.228.1619042624737;\n\tWed, 21 Apr 2021 15:03:44 -0700 (PDT)","Date":"Thu, 22 Apr 2021 00:03:42 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421160319.42251-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16480,"web_url":"https://patchwork.libcamera.org/comment/16480/","msgid":"","date":"2021-04-22T04:53:45","subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the patch,\n\nOn Thu, Apr 22, 2021 at 7:03 AM Niklas Söderlund\n wrote:\n>\n> Hi again Jacopo,\n>\n> On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote:\n> > When a Request is completed upon receiving the IPA produced metadata,\n> > the metadata associated with the Request are over-written, deleting\n> > the information set at output buffer completion, such as the\n> > SensorTimestamp.\n> >\n> > This commit applies to the RkISP1 pipeline handler the same change\n> > applied to IPU3 in commit 13a7ed7b1f1f (\"libcamera: ipu3: Do not\n> > over-write metadata\") but compared to that commit it uses the newly\n> > introduced ControlList::merge() function.\n> >\n> > Fixes: 0eb65e14e18d (\"libcamera: pipeline: rkisp1: Attach to an IPA\")\n>\n> This fixes tag is incorrect. Testing on master the only metadata\n> recorded in the request is the AeLock. Applying this series the only\n> metadata reported is the AeLock and SensorTimestamp, and the timestamp\n> is added by the next patch in this series. There is no regression here\n> to fix, only preparation for additional work, or am I missing something?\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 549f4a4e61a8..c3d390f775f2 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> > if (!info)\n> > return;\n> >\n> > - info->request->metadata() = metadata;\n> > + info->request->metadata().merge(const_cast(metadata));\n> > info->metadataProcessed = true;\n> >\n\n\n> I like the change but this cast makes me think twice of the merge()\n> signature. Could we make the argument a const reference?\n\nI think there are two solutions,\n1.) drop const of action in queueFrameAction.\n2.) merge doesn't change a given ControlList.\n\nWe currently use std::unordered_map::merge() for efficiency, which\ndoesn't copy nodes among maps.\nI expect the copy of ControlValue is basically cheap, but this copy\nmight be expensive depending on the number of nodes to be copied.\nI don't loot at the serialization code during IPA communication, but\nperhaps, the cost between cost reference and value are the same?\nThen, (1) is definitely better in terms of the efficiency.\nI would like to hear others' opinions.\n\n-Hiro\n\n> > pipe->tryCompleteRequest(info->request);\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 E04A7BDB17\n\tfor ;\n\tThu, 22 Apr 2021 04:53:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FCFC6884C;\n\tThu, 22 Apr 2021 06:53:57 +0200 (CEST)","from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com\n\t[IPv6:2a00:1450:4864:20::62b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65EE560516\n\tfor ;\n\tThu, 22 Apr 2021 06:53:56 +0200 (CEST)","by mail-ej1-x62b.google.com with SMTP id w23so50965537ejb.9\n\tfor ;\n\tWed, 21 Apr 2021 21:53:56 -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=\"LvNmBx2n\"; 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:content-transfer-encoding;\n\tbh=l7Wryri18TnLAldHT4sGrfX4BzJzbCQW51QFvYyXaWw=;\n\tb=LvNmBx2nDYOXrbRLVr+LXQhJ0NW/6PeZMLCozETGjoMIqwLYlPJObHKs865J0T1rfn\n\tppVK9NOo/70nBwExgZO0zd+qeeCjPENx+i8vL40GIkoz3FcmhZO2cfE3D0nrHoeou93w\n\tuKuOSpiisOMY14l4qUdqoaRQzZXftIOG+hym0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=l7Wryri18TnLAldHT4sGrfX4BzJzbCQW51QFvYyXaWw=;\n\tb=Q9IuTeHpofr415W05sOzGDNGW6s7/5PoiZyzbVrZSo0Sd7/BWIo+tscrMEPsa/QNEi\n\t7kATvUCeUKA3PZDgdYPvMVmjGl5tY0O+icjbjK2VWoIl0WTR9WwdSpAlpMlKPa3FOkOL\n\tUNBClaWDI6tFV5F70DIfSag6yiNg4UDd51RvwqLL6BWafGX5tnwNHrfRHmICYi50Osaj\n\tpQ1AC9onIwENFGc4h11j86PBp41lZMTRVzaS4wrSD9nkqSTsOyRFFXs1UAo6C1SwCCi3\n\tvnA17ZVfFB82pNaVRxbyHRWCO2m5a5AeFyBF+yDTREQDWQKFxCNsEdxmx8SZ5SKNq0Vp\n\tY75g==","X-Gm-Message-State":"AOAM532gTHd0LrUOWFTB7zd+7btGlOtVHXees/5E9HviDYtqCY2q1/0+\n\t0nt8YvvPZCzkQ9nLjPjudOqSZa0sSTN+Pv35o8fbWA==","X-Google-Smtp-Source":"ABdhPJzCf16RoSYhWbqGZhKgvJKXLjzNBW+vRgHijXKJfnwcV+JIh7MSAA0nqnLhNjDhsi0tvKpEic7U2LmrwLnT//E=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr1395857ejc.292.1619067236097; \n\tWed, 21 Apr 2021 21:53:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-8-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 22 Apr 2021 13:53:45 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16493,"web_url":"https://patchwork.libcamera.org/comment/16493/","msgid":"<20210422070444.3qyw2ykp3yabc5q4@uno.localdomain>","date":"2021-04-22T07:04:44","subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro, Niklas,\n\nOn Thu, Apr 22, 2021 at 01:53:45PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch,\n>\n> On Thu, Apr 22, 2021 at 7:03 AM Niklas Söderlund\n> wrote:\n> >\n> > Hi again Jacopo,\n> >\n> > On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote:\n> > > When a Request is completed upon receiving the IPA produced metadata,\n> > > the metadata associated with the Request are over-written, deleting\n> > > the information set at output buffer completion, such as the\n> > > SensorTimestamp.\n> > >\n> > > This commit applies to the RkISP1 pipeline handler the same change\n> > > applied to IPU3 in commit 13a7ed7b1f1f (\"libcamera: ipu3: Do not\n> > > over-write metadata\") but compared to that commit it uses the newly\n> > > introduced ControlList::merge() function.\n> > >\n> > > Fixes: 0eb65e14e18d (\"libcamera: pipeline: rkisp1: Attach to an IPA\")\n> >\n> > This fixes tag is incorrect. Testing on master the only metadata\n> > recorded in the request is the AeLock. Applying this series the only\n> > metadata reported is the AeLock and SensorTimestamp, and the timestamp\n> > is added by the next patch in this series. There is no regression here\n> > to fix, only preparation for additional work, or am I missing something?\n> >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 549f4a4e61a8..c3d390f775f2 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> > > if (!info)\n> > > return;\n> > >\n> > > - info->request->metadata() = metadata;\n> > > + info->request->metadata().merge(const_cast(metadata));\n> > > info->metadataProcessed = true;\n> > >\n>\n>\n> > I like the change but this cast makes me think twice of the merge()\n> > signature. Could we make the argument a const reference?\n\nWe currently can't as ControlList::merge() is implemented by wrapping\nstd::unordered_map::merge() as Hiro explained\n\n>\n> I think there are two solutions,\n> 1.) drop const of action in queueFrameAction.\n> 2.) merge doesn't change a given ControlList.\n>\n> We currently use std::unordered_map::merge() for efficiency, which\n> doesn't copy nodes among maps.\n> I expect the copy of ControlValue is basically cheap, but this copy\n> might be expensive depending on the number of nodes to be copied.\n\nThe number of nodes and their content, as we can transport Span<> of\nvalues\n\n> I don't loot at the serialization code during IPA communication, but\n> perhaps, the cost between cost reference and value are the same?\n> Then, (1) is definitely better in terms of the efficiency.\n> I would like to hear others' opinions.\n>\n\nI see your concerns about performaces, and in this specific case I\nthink we can safely drop the const from the queueFrameAction\nControlList parameter. Even better, if after merge there are elements\nin the list passed as argument we can catch a development issue\nearlier (an attempt to overwrite a metadata which has been set already).\n\nAnyway a merge() version which takes a const ControlList, and is\nimplemented by copying values might be desirable.\n\nThanks\n j\n\n> -Hiro\n>\n> > > pipe->tryCompleteRequest(info->request);\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 DDB04BDB17\n\tfor ;\n\tThu, 22 Apr 2021 07:04:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 602C868852;\n\tThu, 22 Apr 2021 09:04:07 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB6FD68848\n\tfor ;\n\tThu, 22 Apr 2021 09:04:05 +0200 (CEST)","from uno.localdomain (host-82-57-193-192.retail.telecomitalia.it\n\t[82.57.193.192]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 46546C0002;\n\tThu, 22 Apr 2021 07:04:03 +0000 (UTC)"],"X-Originating-IP":"82.57.193.192","Date":"Thu, 22 Apr 2021 09:04:44 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210422070444.3qyw2ykp3yabc5q4@uno.localdomain>","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-8-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16494,"web_url":"https://patchwork.libcamera.org/comment/16494/","msgid":"<20210422070616.7owgoeqq7aujmdu4@uno.localdomain>","date":"2021-04-22T07:06:16","subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Apr 22, 2021 at 12:03:42AM +0200, Niklas Söderlund wrote:\n> Hi again Jacopo,\n>\n> On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote:\n> > When a Request is completed upon receiving the IPA produced metadata,\n> > the metadata associated with the Request are over-written, deleting\n> > the information set at output buffer completion, such as the\n> > SensorTimestamp.\n> >\n> > This commit applies to the RkISP1 pipeline handler the same change\n> > applied to IPU3 in commit 13a7ed7b1f1f (\"libcamera: ipu3: Do not\n> > over-write metadata\") but compared to that commit it uses the newly\n> > introduced ControlList::merge() function.\n> >\n> > Fixes: 0eb65e14e18d (\"libcamera: pipeline: rkisp1: Attach to an IPA\")\n>\n> This fixes tag is incorrect. Testing on master the only metadata\n> recorded in the request is the AeLock. Applying this series the only\n> metadata reported is the AeLock and SensorTimestamp, and the timestamp\n> is added by the next patch in this series. There is no regression here\n> to fix, only preparation for additional work, or am I missing something?\n>\n\nDunno, I think blindly overwriting metadata is wrong, but if the tag\nmakes you unconfortable I'll drop it.\n\nTbh I don't even know why we use Fixes, since we have nowhere to\nbackport too\n\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 549f4a4e61a8..c3d390f775f2 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> > \tif (!info)\n> > \t\treturn;\n> >\n> > -\tinfo->request->metadata() = metadata;\n> > +\tinfo->request->metadata().merge(const_cast(metadata));\n> > \tinfo->metadataProcessed = true;\n> >\n> > \tpipe->tryCompleteRequest(info->request);\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 079DABDB17\n\tfor ;\n\tThu, 22 Apr 2021 07:05:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C400568852;\n\tThu, 22 Apr 2021 09:05:37 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1BED68847\n\tfor ;\n\tThu, 22 Apr 2021 09:05:35 +0200 (CEST)","from uno.localdomain (host-82-57-193-192.retail.telecomitalia.it\n\t[82.57.193.192]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 204DBC0007;\n\tThu, 22 Apr 2021 07:05:34 +0000 (UTC)"],"X-Originating-IP":"82.57.193.192","Date":"Thu, 22 Apr 2021 09:06:16 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210422070616.7owgoeqq7aujmdu4@uno.localdomain>","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-8-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16500,"web_url":"https://patchwork.libcamera.org/comment/16500/","msgid":"","date":"2021-04-22T07:30:25","subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2021-04-22 09:06:16 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Apr 22, 2021 at 12:03:42AM +0200, Niklas Söderlund wrote:\n> > Hi again Jacopo,\n> >\n> > On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote:\n> > > When a Request is completed upon receiving the IPA produced metadata,\n> > > the metadata associated with the Request are over-written, deleting\n> > > the information set at output buffer completion, such as the\n> > > SensorTimestamp.\n> > >\n> > > This commit applies to the RkISP1 pipeline handler the same change\n> > > applied to IPU3 in commit 13a7ed7b1f1f (\"libcamera: ipu3: Do not\n> > > over-write metadata\") but compared to that commit it uses the newly\n> > > introduced ControlList::merge() function.\n> > >\n> > > Fixes: 0eb65e14e18d (\"libcamera: pipeline: rkisp1: Attach to an IPA\")\n> >\n> > This fixes tag is incorrect. Testing on master the only metadata\n> > recorded in the request is the AeLock. Applying this series the only\n> > metadata reported is the AeLock and SensorTimestamp, and the timestamp\n> > is added by the next patch in this series. There is no regression here\n> > to fix, only preparation for additional work, or am I missing something?\n> >\n> \n> Dunno, I think blindly overwriting metadata is wrong, but if the tag\n> makes you unconfortable I'll drop it.\n\nI agree that overwriting stuff is bad, but the commit in question does \nnot do that, before it there was no metadata at all :-)\n\n> \n> Tbh I don't even know why we use Fixes, since we have nowhere to\n> backport too\n\nI like the Fixes tags. When you bisect issues and hit a bad commit that \nhave a Fixes tag you get way more context on what's going on. That's why \nI think in this particular case the Fixes tag is wrong as if I hit this \ncommit when bisecting the Fixes tag would send me on a wild goose chase \nas this commit does not fix an issue that did not exist in \n0eb65e14e18d~1 but does in 0eb65e14e18d.\n\n> \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 549f4a4e61a8..c3d390f775f2 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> > > \tif (!info)\n> > > \t\treturn;\n> > >\n> > > -\tinfo->request->metadata() = metadata;\n> > > +\tinfo->request->metadata().merge(const_cast(metadata));\n> > > \tinfo->metadataProcessed = true;\n> > >\n> > > \tpipe->tryCompleteRequest(info->request);\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","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 35BAABDB15\n\tfor ;\n\tThu, 22 Apr 2021 07:30:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E628268852;\n\tThu, 22 Apr 2021 09:30:28 +0200 (CEST)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8DA2768847\n\tfor ;\n\tThu, 22 Apr 2021 09:30:27 +0200 (CEST)","by mail-lf1-x12b.google.com with SMTP id n138so70741651lfa.3\n\tfor ;\n\tThu, 22 Apr 2021 00:30:27 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ts10sm182470lfi.184.2021.04.22.00.30.26\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 22 Apr 2021 00:30:26 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"JPUI8Nzv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=MafmFrS/j+c/sSVAohN6KiJNHuaLv1yxPUVh8BzKEU0=;\n\tb=JPUI8NzvRLvOHIbdJTnueM1oaOC/UwLE/jya77lDhxZZTaaquX8b8C3/hkmNlhSoBC\n\tfhcCuAJ1uhZZYlv64kDJ4LylvuohFhqIucZU7iNdmaV96Dvq6uRHt/yLs0tPW9AJbQ3V\n\t9QcQNeQqDFHv1xF/V5uKohVtC0qyL4PmrtwhzZSGQ6vIx74NUEI0aM6lypsiDVRJGN24\n\to9N+ldlYBja7KK4bomD+SY/S3NNbTAL3n5qxPFFm/P0r9sBUpZKLQegvyCacbiNuHk/v\n\t099EzsgyvHoD4jOijWM+dBgqJ9Y9VX5NWAHxH+066JYmKxvExBfGa3o2C+9BoS1ZK73s\n\to6Tg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=MafmFrS/j+c/sSVAohN6KiJNHuaLv1yxPUVh8BzKEU0=;\n\tb=L5DpXLalbWsI2iBKR3EMVyN+ocxRgS3Sr7dvRvE3j9aEcuKOokYRK4cKgMrKA3no7P\n\t2GRyDaetl+wv+GAP8B/bxMUIVKh+bBuWUU+5XCE+JASOSPAtMulQfMj425VGv3Qzl6Ju\n\t/KBqinA4XIKlygA+WDPp5r0uRh9JURHAQbGw9RqBp+LDHKwnh6NEamtAP4s+Ei50khyY\n\tVOfX7BKZN6Pu7JmSepepUKYRfNm2HBeq13cbOoQD/zDgzj+HoNB9Aptb87U45tY5P3US\n\tooUz6PKA2Y9PyYjVb6UT+gDR4096BfU3wruGLus8EkiTl+z9D3Tns+QwXQUn0kH5CSOS\n\twggA==","X-Gm-Message-State":"AOAM5333gX8EoWX430h7QHlTVxZXoAMhRaATYQr2BDSKSrsFJFgD7ZUq\n\tCdM+QEGFfrHN2kYdKjlQqDmGHYRZjNRd+YiD","X-Google-Smtp-Source":"ABdhPJzZvUHMgFVZr340I3AFMunwzhYqQNwCpCcIKcS1UyU62eO5Cq1kkrrHk06WV5ZErOcE5eZ3vg==","X-Received":"by 2002:ac2:4e84:: with SMTP id o4mr1471701lfr.557.1619076627008;\n\tThu, 22 Apr 2021 00:30:27 -0700 (PDT)","Date":"Thu, 22 Apr 2021 09:30:25 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-8-jacopo@jmondi.org>\n\t\n\t<20210422070616.7owgoeqq7aujmdu4@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422070616.7owgoeqq7aujmdu4@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16560,"web_url":"https://patchwork.libcamera.org/comment/16560/","msgid":"","date":"2021-04-26T04:59:20","subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Apr 22, 2021 at 09:04:44AM +0200, Jacopo Mondi wrote:\n> On Thu, Apr 22, 2021 at 01:53:45PM +0900, Hirokazu Honda wrote:\n> > On Thu, Apr 22, 2021 at 7:03 AM Niklas Söderlund wrote:\n> > > On 2021-04-21 18:03:10 +0200, Jacopo Mondi wrote:\n> > > > When a Request is completed upon receiving the IPA produced metadata,\n> > > > the metadata associated with the Request are over-written, deleting\n> > > > the information set at output buffer completion, such as the\n> > > > SensorTimestamp.\n> > > >\n> > > > This commit applies to the RkISP1 pipeline handler the same change\n> > > > applied to IPU3 in commit 13a7ed7b1f1f (\"libcamera: ipu3: Do not\n> > > > over-write metadata\") but compared to that commit it uses the newly\n> > > > introduced ControlList::merge() function.\n> > > >\n> > > > Fixes: 0eb65e14e18d (\"libcamera: pipeline: rkisp1: Attach to an IPA\")\n> > >\n> > > This fixes tag is incorrect. Testing on master the only metadata\n> > > recorded in the request is the AeLock. Applying this series the only\n> > > metadata reported is the AeLock and SensorTimestamp, and the timestamp\n> > > is added by the next patch in this series. There is no regression here\n> > > to fix, only preparation for additional work, or am I missing something?\n> > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 549f4a4e61a8..c3d390f775f2 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -362,7 +362,7 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> > > > if (!info)\n> > > > return;\n> > > >\n> > > > - info->request->metadata() = metadata;\n> > > > + info->request->metadata().merge(const_cast(metadata));\n> > > > info->metadataProcessed = true;\n> > >\n> > > I like the change but this cast makes me think twice of the merge()\n> > > signature. Could we make the argument a const reference?\n\nOuch. const_cast<> is very often a sign of a design issue.\n\n> We currently can't as ControlList::merge() is implemented by wrapping\n> std::unordered_map::merge() as Hiro explained\n> \n> > I think there are two solutions,\n> > 1.) drop const of action in queueFrameAction.\n> > 2.) merge doesn't change a given ControlList.\n> >\n> > We currently use std::unordered_map::merge() for efficiency, which\n> > doesn't copy nodes among maps.\n> > I expect the copy of ControlValue is basically cheap, but this copy\n> > might be expensive depending on the number of nodes to be copied.\n> \n> The number of nodes and their content, as we can transport Span<> of\n> values\n> \n> > I don't loot at the serialization code during IPA communication, but\n> > perhaps, the cost between cost reference and value are the same?\n> > Then, (1) is definitely better in terms of the efficiency.\n> > I would like to hear others' opinions.\n> \n> I see your concerns about performaces, and in this specific case I\n> think we can safely drop the const from the queueFrameAction\n> ControlList parameter. Even better, if after merge there are elements\n> in the list passed as argument we can catch a development issue\n> earlier (an attempt to overwrite a metadata which has been set already).\n\nI also think this is better, and I like the idea of being able to check\nif metadata is empty after the merge. You'll run into an issue though,\nas the IPA IPC code generator inserts the const automatically. To avoid\ndelay in this patch series, I'd recommend modifying ControlList::merge()\nto take a const reference and copy elements, with a \\todo to mention we\nshould optimize it.\n\n> Anyway a merge() version which takes a const ControlList, and is\n> implemented by copying values might be desirable.\n> \n> > > > pipe->tryCompleteRequest(info->request);","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 687F4BDC91\n\tfor ;\n\tMon, 26 Apr 2021 04:59:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D26B4688A7;\n\tMon, 26 Apr 2021 06:59:27 +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 14507605BD\n\tfor ;\n\tMon, 26 Apr 2021 06:59:26 +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 728624FB;\n\tMon, 26 Apr 2021 06:59:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"U4ttqq/7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619413165;\n\tbh=Wj8uYCjLTrHeQR1IakO1iGvKKf6smJYitcRpIw8gMrQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U4ttqq/74KKcu0kxHkPJe0jbBrMYUaI9hql6NYC0AjzIbj58rOfDmBZ5n65Vtu0K0\n\t/aBmoPrgrtC/Vn20ecnKUYAmt135QttmCGccsfw0uOrcgYyAMCgUDTjQy/IiDiXHdd\n\toSP2kmvGGzU2oP1Rwo/uhqURpSDumAslypxVVoRc=","Date":"Mon, 26 Apr 2021 07:59:20 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-8-jacopo@jmondi.org>\n\t\n\t\n\t<20210422070444.3qyw2ykp3yabc5q4@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422070444.3qyw2ykp3yabc5q4@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 07/16] libcamera: rkisp1: Do not\n\tover-write metadata","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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]