[{"id":16467,"web_url":"https://patchwork.libcamera.org/comment/16467/","msgid":"","date":"2021-04-21T18:47:45","subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","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-21 18:03:14 +0200, Jacopo Mondi wrote:\n> I got fooled by the documentation of setRequest() implying that the\n> function is meant to be called by pipeline handlers only, which it is\n> used in the Request class at Request::addBuffer() and Request::reuse()\n> time.\n> \n> Rework the documentation to report that.\n> \n> Fixes: 125be3436ae0 (\"libcamera: FrameBuffer: Add a setRequest() interface\")\n\nIn all fairness this was true when 125be3436ae0 was merged it only \nchanged in dcc024760a8d that was merged a year later ;-)\n\n> Reviewed-by: Laurent Pinchart \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/buffer.cpp | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 75b2693281a7..5baccfa99f10 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> * \\brief Set the request this buffer belongs to\n> * \\param[in] request Request to set\n> *\n> - * The intended callers of this method are pipeline handlers and only for\n> - * buffers that are internal to the pipeline.\n> + * For buffers added to requests by applications, this method is called by\n> + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline\n> + * handlers, it is called by the pipeline handlers themselves.\n> *\n> * \\todo Shall be hidden from applications with a d-pointer design.\n> */\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 ECD49BDB15\n\tfor ;\n\tWed, 21 Apr 2021 18:47:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A4FE6884B;\n\tWed, 21 Apr 2021 20:47:49 +0200 (CEST)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81DC868840\n\tfor ;\n\tWed, 21 Apr 2021 20:47:47 +0200 (CEST)","by mail-lf1-x132.google.com with SMTP id t14so11186717lfe.1\n\tfor ;\n\tWed, 21 Apr 2021 11:47:47 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id z9sm38698lfd.2.2021.04.21.11.47.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 21 Apr 2021 11:47:46 -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=\"fBCOndGd\"; 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=V52rk+saUZZvHDirQBCVdTF0pY+x1SatY4atA/5mKI8=;\n\tb=fBCOndGdmcXi+u36l5o4QPzgFlMXiuyWwkAFx1WOAG5BNC66pq7dQswUotrOgLkcS3\n\tlr3HBs+9/eGZSG4Nf6LNzNVRwjOjLlUEcYt1owHnF/FKlgWdgNzcGWmCyS9mhVQt9w0i\n\t1gD8SPlMgmXjzl7pGac67fFg5hveXjdSuf6CELzmx5qMmoVydBBiWY3/9JcWrOOmVNbg\n\tL38G3vb8J4NvsNNIBBMbN2C006cxUuwvX3rGdQ4Sx/UUDtCc0C72D7Ei2/lYIwNBTtQC\n\t/HfCo9he49QdNA+iH5X6iv8QFPH9tCVQo5haExnD5GW8VUjNvx/csbTHGwNj6CrzHg6y\n\tdXog==","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=V52rk+saUZZvHDirQBCVdTF0pY+x1SatY4atA/5mKI8=;\n\tb=WdDZoMZNUNyUUqGyhnl5HyeMbywS66BZkvPPIh7K9duleGxJnadoAjN1fCp9FFaM8L\n\t9rRmQTyWhG14ylWTTXXeJyT34+PCToDXPY2rkrIMp/BmhTrbk221AEy089WUqkrFZ3LX\n\tq+0bbv7ywME09jT1Qm7wsa5h+Ziko9qXt8bhR9gMJ3IR/u8bzT7NctSV6KsTs6Tx3lo8\n\tNbPnAlp12XHhtYwrNvSpMk0s8zPV17v2rmin00jL277GfCmyVl9RPPU/GLPhw4J33e2T\n\t+bH5HMpvc6k4A0qoHJ4uYjHURt5AUWSEL0hbMerQtux7WAC/ofZC/uuAcQQ131M1Ml5I\n\t6pbA==","X-Gm-Message-State":"AOAM531hrnXzXVA9CNUXdnJdp+VcbAf7iWw699iui8AVMVbYoHivpeqw\n\tZ9gsYJYWPNjIEsQA7CRumRovhmzkA120te4z","X-Google-Smtp-Source":"ABdhPJxYaNUZ6z2g8ww+pFH0tqxMHXp0sHGUZ+VCz9AtAZbAvow3u/wZp/4xcQs8miuRNQgkE3G45w==","X-Received":"by 2002:a19:dcd:: with SMTP id\n\t196mr20196068lfn.190.1619030866922; \n\tWed, 21 Apr 2021 11:47:46 -0700 (PDT)","Date":"Wed, 21 Apr 2021 20:47:45 +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-12-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421160319.42251-12-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","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":16484,"web_url":"https://patchwork.libcamera.org/comment/16484/","msgid":"","date":"2021-04-22T05:07:12","subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","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 3:47 AM Niklas Söderlund\n wrote:\n>\n> Hi Jacopo,\n>\n> On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote:\n> > I got fooled by the documentation of setRequest() implying that the\n> > function is meant to be called by pipeline handlers only, which it is\n> > used in the Request class at Request::addBuffer() and Request::reuse()\n> > time.\n> >\n> > Rework the documentation to report that.\n> >\n> > Fixes: 125be3436ae0 (\"libcamera: FrameBuffer: Add a setRequest() interface\")\n>\n> In all fairness this was true when 125be3436ae0 was merged it only\n> changed in dcc024760a8d that was merged a year later ;-)\n>\n> > Reviewed-by: Laurent Pinchart \n> > Signed-off-by: Jacopo Mondi \n>\n> Reviewed-by: Niklas Söderlund \n>\n> > ---\n> > src/libcamera/buffer.cpp | 5 +++--\n> > 1 file changed, 3 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 75b2693281a7..5baccfa99f10 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > * \\brief Set the request this buffer belongs to\n> > * \\param[in] request Request to set\n> > *\n> > - * The intended callers of this method are pipeline handlers and only for\n> > - * buffers that are internal to the pipeline.\n> > + * For buffers added to requests by applications, this method is called by\n> > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline\n> > + * handlers, it is called by the pipeline handlers themselves.\n> > *\n> > * \\todo Shall be hidden from applications with a d-pointer design.\n> > */\n\nIs this todo still reasonable? It needs to be public so that a\npipeline handler calls it?\n\n> > --\n> > 2.31.1\n> >\n\nReviewed-by: Hirokazu Honda \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 6A920BDB17\n\tfor ;\n\tThu, 22 Apr 2021 05:07:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9CC86884C;\n\tThu, 22 Apr 2021 07:07:24 +0200 (CEST)","from mail-ej1-x630.google.com (mail-ej1-x630.google.com\n\t[IPv6:2a00:1450:4864:20::630])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F7A86884B\n\tfor ;\n\tThu, 22 Apr 2021 07:07:23 +0200 (CEST)","by mail-ej1-x630.google.com with SMTP id w3so66846711ejc.4\n\tfor ;\n\tWed, 21 Apr 2021 22:07:23 -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=\"TLQIcAef\"; 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=NaINqbqAKzUemDdA56YybE+j7u0sY0m/kJJC9wSFoBs=;\n\tb=TLQIcAefwFMuY/r7DCrfHq3Sc28INib/muKusrVYfEr6YMgHnC+dAizi7QkHpysYFq\n\tSEnaQiYoSWdC8ZBA5cNajVbjDAxNyAKtmrn1oIvgyeOoGPr763cUAUQNcoOjSvMxVbiK\n\tffhTTbdoePieMvr/bE9Io70ToPGc6UZj4/YeQ=","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=NaINqbqAKzUemDdA56YybE+j7u0sY0m/kJJC9wSFoBs=;\n\tb=e2dlx2VZeX/EjPOyJXL06Gmp3zc5MYl+u4mmXTr/XJ8kCXkfQ3Gx+cHFXy/bW07nkY\n\tJX0tsMK3Z619eedQaRZFJWycLHpEZ4KEWrQCAweUVScjkuVaTfOmFkUN5EPUfctd46rA\n\t6GHq9kE8PgPnLFBBy2b3gMIioOALhi6nGPYFnvpBOITQq6p65GiJOSB4VagXRkLPoD0A\n\t2v56g2QmLRJoFuau91ZfbMzpJcDpC69Kf5fiXaTndWMoZw2cs/ffyxQ3BoFLrvXv/XtN\n\t56alWIlKistUVlcqag0F5WM5R58P5BwfBi4/AT/ZGJF3tk1HfGkxtVl6aBtRRAAAUTRW\n\t5iSw==","X-Gm-Message-State":"AOAM533VPwIfmjTe+hLNjbmTTTDG8Hx48h6pOQq1OumNnGnVgE5nrGJt\n\tAxpNSvGUMfnHQjB/EZH5ZbYKGyVKMUF56r/vWPPNDw==","X-Google-Smtp-Source":"ABdhPJxPcUramUsbBWt5fgBWa73BfNuuWFMJbKAn7tgJ9KUAl3DPFH6uwrncC5Qw4JKw/tOwob+kgQ1kbdyqqsZ/Vmc=","X-Received":"by 2002:a17:906:349b:: with SMTP id\n\tg27mr1387459ejb.306.1619068043246; \n\tWed, 21 Apr 2021 22:07:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-12-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 22 Apr 2021 14:07:12 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","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":16495,"web_url":"https://patchwork.libcamera.org/comment/16495/","msgid":"<20210422071050.xh5rthmmxi2vbxay@uno.localdomain>","date":"2021-04-22T07:10:50","subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Apr 22, 2021 at 02:07:12PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch.\n>\n> On Thu, Apr 22, 2021 at 3:47 AM Niklas Söderlund\n> wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote:\n> > > I got fooled by the documentation of setRequest() implying that the\n> > > function is meant to be called by pipeline handlers only, which it is\n> > > used in the Request class at Request::addBuffer() and Request::reuse()\n> > > time.\n> > >\n> > > Rework the documentation to report that.\n> > >\n> > > Fixes: 125be3436ae0 (\"libcamera: FrameBuffer: Add a setRequest() interface\")\n> >\n> > In all fairness this was true when 125be3436ae0 was merged it only\n> > changed in dcc024760a8d that was merged a year later ;-)\n> >\n> > > Reviewed-by: Laurent Pinchart \n> > > Signed-off-by: Jacopo Mondi \n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> > > ---\n> > > src/libcamera/buffer.cpp | 5 +++--\n> > > 1 file changed, 3 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > index 75b2693281a7..5baccfa99f10 100644\n> > > --- a/src/libcamera/buffer.cpp\n> > > +++ b/src/libcamera/buffer.cpp\n> > > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > * \\brief Set the request this buffer belongs to\n> > > * \\param[in] request Request to set\n> > > *\n> > > - * The intended callers of this method are pipeline handlers and only for\n> > > - * buffers that are internal to the pipeline.\n> > > + * For buffers added to requests by applications, this method is called by\n> > > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline\n> > > + * handlers, it is called by the pipeline handlers themselves.\n> > > *\n> > > * \\todo Shall be hidden from applications with a d-pointer design.\n> > > */\n>\n> Is this todo still reasonable? It needs to be public so that a\n> pipeline handler calls it?\n\nI think \"public\" meant \"visible from application\"\n\nProbablya the todo has to be read as:\n\n- We need to implement d-pointer (or PIMPL, or whatever, I'm not happy\n we deviated from C++ to adhere to a naming convention introduced by another\n framework such as Qt, but...) not to expose methods that are intentended\n to be used by library internal components (such as PH or the Request\n class) to applications.\n\n>\n> > > --\n> > > 2.31.1\n> > >\n>\n> Reviewed-by: Hirokazu Honda \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 40C46BDB15\n\tfor ;\n\tThu, 22 Apr 2021 07:10:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C90668852;\n\tThu, 22 Apr 2021 09:10:11 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C185668847\n\tfor ;\n\tThu, 22 Apr 2021 09:10:10 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id 664A9100011;\n\tThu, 22 Apr 2021 07:10:09 +0000 (UTC)"],"Date":"Thu, 22 Apr 2021 09:10:50 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210422071050.xh5rthmmxi2vbxay@uno.localdomain>","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-12-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","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":16497,"web_url":"https://patchwork.libcamera.org/comment/16497/","msgid":"","date":"2021-04-22T07:26:47","subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Thu, Apr 22, 2021 at 4:10 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Apr 22, 2021 at 02:07:12PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the patch.\n> >\n> > On Thu, Apr 22, 2021 at 3:47 AM Niklas Söderlund\n> > wrote:\n> > >\n> > > Hi Jacopo,\n> > >\n> > > On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote:\n> > > > I got fooled by the documentation of setRequest() implying that the\n> > > > function is meant to be called by pipeline handlers only, which it is\n> > > > used in the Request class at Request::addBuffer() and Request::reuse()\n> > > > time.\n> > > >\n> > > > Rework the documentation to report that.\n> > > >\n> > > > Fixes: 125be3436ae0 (\"libcamera: FrameBuffer: Add a setRequest() interface\")\n> > >\n> > > In all fairness this was true when 125be3436ae0 was merged it only\n> > > changed in dcc024760a8d that was merged a year later ;-)\n> > >\n> > > > Reviewed-by: Laurent Pinchart \n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > > > ---\n> > > > src/libcamera/buffer.cpp | 5 +++--\n> > > > 1 file changed, 3 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > > index 75b2693281a7..5baccfa99f10 100644\n> > > > --- a/src/libcamera/buffer.cpp\n> > > > +++ b/src/libcamera/buffer.cpp\n> > > > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > > * \\brief Set the request this buffer belongs to\n> > > > * \\param[in] request Request to set\n> > > > *\n> > > > - * The intended callers of this method are pipeline handlers and only for\n> > > > - * buffers that are internal to the pipeline.\n> > > > + * For buffers added to requests by applications, this method is called by\n> > > > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline\n> > > > + * handlers, it is called by the pipeline handlers themselves.\n> > > > *\n> > > > * \\todo Shall be hidden from applications with a d-pointer design.\n> > > > */\n> >\n> > Is this todo still reasonable? It needs to be public so that a\n> > pipeline handler calls it?\n>\n> I think \"public\" meant \"visible from application\"\n>\n> Probablya the todo has to be read as:\n>\n> - We need to implement d-pointer (or PIMPL, or whatever, I'm not happy\n> we deviated from C++ to adhere to a naming convention introduced by another\n> framework such as Qt, but...) not to expose methods that are intentended\n> to be used by library internal components (such as PH or the Request\n> class) to applications.\n>\n\nPipelineHandler and Request cannot call a private function of\nFrameBuffer and a function of FrameBuffer::Private, can they?\nD-pointer doesn't help for hiding the function as PH, Request and\nApplication are the same level from FrameBuffer.\nI this the possible way is to move setRequest to private and make PH\nand Request friends. But I think it is not a right direction..\nI am honestly have no idea how to hide from applications keeping them\naccessible from PH and Request.\nI would drop \"with d-pointer design\".\nI may miss something. Appreciate if you correct me.\n\nThanks,\n-Hiro\n\n\n> >\n> > > > --\n> > > > 2.31.1\n> > > >\n> >\n> > Reviewed-by: Hirokazu Honda \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 C9196BDB15\n\tfor ;\n\tThu, 22 Apr 2021 07:26:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 304DD68854;\n\tThu, 22 Apr 2021 09:26:59 +0200 (CEST)","from mail-ej1-x632.google.com (mail-ej1-x632.google.com\n\t[IPv6:2a00:1450:4864:20::632])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FE5568847\n\tfor ;\n\tThu, 22 Apr 2021 09:26:58 +0200 (CEST)","by mail-ej1-x632.google.com with SMTP id u17so67293037ejk.2\n\tfor ;\n\tThu, 22 Apr 2021 00:26:58 -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=\"RP5lcGNF\"; 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=GmRNvT8M7hdxCwhferh+tFdDY/ovG4vQkAf1c/PaCAM=;\n\tb=RP5lcGNFGhdbE2IhXejkLnv+xMk872wzUb40eoXCLS7SZGMm8u2mFNCADATWebkW5L\n\tKz/N+RwUtZ+pSGstPzOLwA03CfYNnr2/W+uf+RzgaAEL8lIQU6BtSM/NZ7rftSEl5YQF\n\tMeSAMFudo6cTjUZlh9yZXTEUVaRaP75LIrPBQ=","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=GmRNvT8M7hdxCwhferh+tFdDY/ovG4vQkAf1c/PaCAM=;\n\tb=PymGghRk9qzSpE0qLXFxjo2ClNRcqAm4Y8uIMpKkR7s0PQdCjYqsnB/IZLwMo5NJBA\n\tr0hdjpcACcMeYRk7L30bivlMJLfjQBj/F/QWRRRuO7S8d94qiTfnOlS7IEmMMJLZSGoH\n\t8bI1aUFoiWJD4DMM1FZGKVfpzHeAifP6hgy/cNpX3OW4Ev8xvNIk9CQmC2tTFstLn1zH\n\te6e540lli2fCsfeuzdWD7rWmJVOtOMFHt9nD+B7oLkKQZRQZfY7Fa4Ridu5wq/mygU30\n\twVppKI3Ny9kn7R1a3EGV8zTlMUbSkrc6NXUYKT57GDT25IrgYhsvfKJIzCH1xGNhLo91\n\tciwQ==","X-Gm-Message-State":"AOAM532i+XLLCwwu51XKorB4CyutMjy5nFRg53Yuy8WSlXZf7md7fkgq\n\t2dJs/ASDMTs4I1KRqld7Cuic0uBY+2upuJUwRGOx4IySyFk=","X-Google-Smtp-Source":"ABdhPJx+yTi1GdEx6Un1LmVE9qqrFWeMp2/4g9Yl1zvp1+wYCZr4E/Y/O/sYAizZtBG7bEXLdQyD4vPKVGRVcVxFYAk=","X-Received":"by 2002:a17:906:fcc4:: with SMTP id\n\tqx4mr1871647ejb.42.1619076417864; \n\tThu, 22 Apr 2021 00:26:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-12-jacopo@jmondi.org>\n\t\n\t\n\t<20210422071050.xh5rthmmxi2vbxay@uno.localdomain>","In-Reply-To":"<20210422071050.xh5rthmmxi2vbxay@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 22 Apr 2021 16:26:47 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","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":16559,"web_url":"https://patchwork.libcamera.org/comment/16559/","msgid":"","date":"2021-04-26T04:45:13","subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello Hiro and Jacopo,\n\nOn Thu, Apr 22, 2021 at 04:26:47PM +0900, Hirokazu Honda wrote:\n> On Thu, Apr 22, 2021 at 4:10 PM Jacopo Mondi wrote:\n> > On Thu, Apr 22, 2021 at 02:07:12PM +0900, Hirokazu Honda wrote:\n> > > On Thu, Apr 22, 2021 at 3:47 AM Niklas Söderlund wrote:\n> > > > On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote:\n> > > > > I got fooled by the documentation of setRequest() implying that the\n> > > > > function is meant to be called by pipeline handlers only, which it is\n> > > > > used in the Request class at Request::addBuffer() and Request::reuse()\n> > > > > time.\n> > > > >\n> > > > > Rework the documentation to report that.\n> > > > >\n> > > > > Fixes: 125be3436ae0 (\"libcamera: FrameBuffer: Add a setRequest() interface\")\n> > > >\n> > > > In all fairness this was true when 125be3436ae0 was merged it only\n> > > > changed in dcc024760a8d that was merged a year later ;-)\n> > > >\n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > Signed-off-by: Jacopo Mondi \n> > > >\n> > > > Reviewed-by: Niklas Söderlund \n> > > >\n> > > > > ---\n> > > > > src/libcamera/buffer.cpp | 5 +++--\n> > > > > 1 file changed, 3 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > > > > index 75b2693281a7..5baccfa99f10 100644\n> > > > > --- a/src/libcamera/buffer.cpp\n> > > > > +++ b/src/libcamera/buffer.cpp\n> > > > > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > > > * \\brief Set the request this buffer belongs to\n> > > > > * \\param[in] request Request to set\n> > > > > *\n> > > > > - * The intended callers of this method are pipeline handlers and only for\n> > > > > - * buffers that are internal to the pipeline.\n> > > > > + * For buffers added to requests by applications, this method is called by\n> > > > > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline\n> > > > > + * handlers, it is called by the pipeline handlers themselves.\n> > > > > *\n> > > > > * \\todo Shall be hidden from applications with a d-pointer design.\n> > > > > */\n> > >\n> > > Is this todo still reasonable? It needs to be public so that a\n> > > pipeline handler calls it?\n> >\n> > I think \"public\" meant \"visible from application\"\n> >\n> > Probablya the todo has to be read as:\n> >\n> > - We need to implement d-pointer (or PIMPL, or whatever, I'm not happy\n> > we deviated from C++ to adhere to a naming convention introduced by another\n> > framework such as Qt, but...)\n\nNote that this design pattern is known by different names\n(https://en.wikipedia.org/wiki/Opaque_pointer), and the name pimpl isn't\npart of the C++ standard as far as I know. I'm fine using a different\nname than d-pointer though, if one of the other ones is more common (I\nmay veto Cheshire cat though :-)).\n\n> > not to expose methods that are intentended\n> > to be used by library internal components (such as PH or the Request\n> > class) to applications.\n> \n> PipelineHandler and Request cannot call a private function of\n> FrameBuffer and a function of FrameBuffer::Private, can they?\n> D-pointer doesn't help for hiding the function as PH, Request and\n> Application are the same level from FrameBuffer.\n> I this the possible way is to move setRequest to private and make PH\n> and Request friends. But I think it is not a right direction..\n> I am honestly have no idea how to hide from applications keeping them\n> accessible from PH and Request.\n> I would drop \"with d-pointer design\".\n> I may miss something. Appreciate if you correct me.\n\nWith this design pattern, the FrameBuffer::Private class can be defined\nin an *internal* header file. The class can then define public functions\nexposed to other classes. We would need to make the Extensible::_d()\nfunctions public (they're currently protected), which I don't think\nwould be a problem. Applications could call _d() and get a pointer to\nthe Private object, but given that the Private class would be defined in\nan internal header, they wouldn't be able to access it (and even if they\ndid, they couldn't claim in good faith they didn't know it wasn't a\npublic API :-)).\n\n> > > Reviewed-by: Hirokazu Honda ","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 6F02EBDC92\n\tfor ;\n\tMon, 26 Apr 2021 04:45:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9E53688A1;\n\tMon, 26 Apr 2021 06:45:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D14A605BD\n\tfor ;\n\tMon, 26 Apr 2021 06:45:19 +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 ACD694FB;\n\tMon, 26 Apr 2021 06:45:18 +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=\"NlZRntK2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619412318;\n\tbh=PZMl6J691YBrokgFQCXD4Ihiu1a8pSyrjeo19ImnpKU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NlZRntK2D3DwcvexEyvLDpmmawbp42tDv1gEL4An2IBC0HT0n8SmehAgLZesAtAsX\n\tV4kiqpSDRFMyoQWRqBIUAyt9vMw9EHzfhCegCFyTeWig+Ve9kLJppYx3NLYvBjsIPu\n\tqwYpNmEdjiEpwA8iZBFdiUlM2ZT6mer079gc9J+g=","Date":"Mon, 26 Apr 2021 07:45:13 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-12-jacopo@jmondi.org>\n\t\n\t\n\t<20210422071050.xh5rthmmxi2vbxay@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work\n\tsetRequest() documentation","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\" "}}]