[{"id":20404,"web_url":"https://patchwork.libcamera.org/comment/20404/","msgid":"","date":"2021-10-25T04:53:53","subject":"Re: [libcamera-devel] [PATCH v6 1/7] android: camera_stream:\n\tReplace post-processor nullptr check","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Sat, Oct 23, 2021 at 04:02:56PM +0530, Umang Jain wrote:\n> Instead of checking postProcessor for nullptr, replace this\n> check with an assertion that checks if the camera stream's\n> type is not Type::Direct. Since it makes no sense to call\n> CameraStream::process() on a Type::Direct camera stream.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_stream.cpp | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index f44a2717..ca25c4db 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -163,8 +163,7 @@ int CameraStream::process(const FrameBuffer &source,\n> \t\tdest.fence = -1;\n> \t}\n> \n> -\tif (!postProcessor_)\n> -\t\treturn 0;\n> +\tASSERT(type_ != Type::Direct);\n\nI wonder if this is really equivalent. It depends what the goal of the\ncheck was. I've understood is as protecting against configure() not\nhaving been called successfully, but that's just my interpretation, it\ncould have been there to protect against process() being called on\ndirect streams. As both are equally unlikely, I'm fine with the change.\nCould you however move this to the beginning of the function ?\n\nReviewed-by: Laurent Pinchart \n\n> \n> \t/*\n> \t * \\todo Buffer mapping and processing should be moved to a","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 1AAB1BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 04:54:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D99564870;\n\tMon, 25 Oct 2021 06:54:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2725960237\n\tfor ;\n\tMon, 25 Oct 2021 06:54:15 +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 8D0BEE0A;\n\tMon, 25 Oct 2021 06:54: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=\"UK/nZgs2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635137654;\n\tbh=NX+Ng30Y+giypZYcH1Inoo7aZ/+VHq1mXLqtpmVX3Ks=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UK/nZgs20gC7R5bQV8nZbSiCxffJBc6t9JvTEqWVuigRdbA3ssh61NR3XymkOdSbW\n\tA9P0NGZA5FTV1xrmh69uvNRaUGTtQMoVPtE+kZOWYlW6LhXjh0pM+Imtu502Y9jQda\n\t12Ffp7CT7KrtsfTVfKa3L9uqxDsuoG8QgaKPx8ek=","Date":"Mon, 25 Oct 2021 07:53:53 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211023103302.152769-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] android: camera_stream:\n\tReplace post-processor nullptr check","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":20407,"web_url":"https://patchwork.libcamera.org/comment/20407/","msgid":"","date":"2021-10-25T05:15:41","subject":"Re: [libcamera-devel] [PATCH v6 1/7] android: camera_stream:\n\tReplace post-processor nullptr check","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Mon, Oct 25, 2021 at 1:54 PM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Sat, Oct 23, 2021 at 04:02:56PM +0530, Umang Jain wrote:\n> > Instead of checking postProcessor for nullptr, replace this\n> > check with an assertion that checks if the camera stream's\n> > type is not Type::Direct. Since it makes no sense to call\n> > CameraStream::process() on a Type::Direct camera stream.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_stream.cpp | 3 +--\n> > 1 file changed, 1 insertion(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index f44a2717..ca25c4db 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -163,8 +163,7 @@ int CameraStream::process(const FrameBuffer &source,\n> > dest.fence = -1;\n> > }\n> >\n> > - if (!postProcessor_)\n> > - return 0;\n> > + ASSERT(type_ != Type::Direct);\n>\n> I wonder if this is really equivalent. It depends what the goal of the\n> check was. I've understood is as protecting against configure() not\n> having been called successfully, but that's just my interpretation, it\n> could have been there to protect against process() being called on\n> direct streams. As both are equally unlikely, I'm fine with the change.\n> Could you however move this to the beginning of the function ?\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nI think this is satisfied at this moment. Although we may have to\nchange this while we are supporting more post processing cases.\n\nReviewed-by: Hirokazu Honda \n\n> >\n> > /*\n> > * \\todo Buffer mapping and processing should be moved to a\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 C8A35BF415\n\tfor ;\n\tMon, 25 Oct 2021 05:15:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 138E464870;\n\tMon, 25 Oct 2021 07:15:54 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 256BF60237\n\tfor ;\n\tMon, 25 Oct 2021 07:15:53 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id r12so5202471edt.6\n\tfor ;\n\tSun, 24 Oct 2021 22:15:53 -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=\"F178GaY0\"; 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=RX+L07ksXuiq2nBB5hQhpT/pMdYvltELAIi7p1jj7CM=;\n\tb=F178GaY0yVz6WJEY+S+d+XvIO90ljk0k3Vd+y5v8kbc5hwDB8fF/IJgVc0RxovTs+G\n\tU80B2T8CdZV3U64aaJAd+2N+k1oBSkEBqmFh0sn3E4gx34y36rH7DiRA4ycgyLyK6s4H\n\t0NEiOjAWNErWUt3ISXgvm9njHpFF0ZyPoWcug=","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=RX+L07ksXuiq2nBB5hQhpT/pMdYvltELAIi7p1jj7CM=;\n\tb=wZ6vQdCYhKhqvoUb28vLmFnMJCxEjyLr883jsmWGlL71AkwyBc12vGxvBCLQiM9sWL\n\tu9PgukykulRM2soHN/8oCRfiJfER3zNe49H9Af0oDM1sG5+AaoinLNIo3FPtjUGikf3R\n\tPXNqBL/xCXMnkubRVLlKUNB4sym2L3CY1Tzi/JxNWV7JXvqBV7MqXHwBYaZoWF5TKZTi\n\tus9GYeLFOsUiRUODzH/6Azqy5AYD9T+E/3O24EeeN6g8zeUKCIDXhvumHyqlrDgY78LF\n\todg43ylxSlrBXDCkXq3FxKLhCPcpx6Dwhv4a0kP/QxH1A5+ZSDjBNANrJfzCO5SvZANR\n\tsPcw==","X-Gm-Message-State":"AOAM531NJ11Sp+INKSR6zvxXvkJ0CaJWslExLtqVYcNbxmCGmXWqUrvR\n\tVhJQsXXibiOQvbaSrK/U6GypSmn2bciMokb42KcdBN/cW3s=","X-Google-Smtp-Source":"ABdhPJzSZgLHsw6YWCEQrGcRToW8vkIA+buf6bUZJdDXqfY1JcWZ997RRkaog6rhsl9m9AIc0JM9tAxQQ99VNowRG4o=","X-Received":"by 2002:a17:907:7ba6:: with SMTP id\n\tne38mr19679548ejc.150.1635138952736; \n\tSun, 24 Oct 2021 22:15:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-2-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 25 Oct 2021 14:15:41 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] android: camera_stream:\n\tReplace post-processor nullptr check","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\" "}},{"id":20444,"web_url":"https://patchwork.libcamera.org/comment/20444/","msgid":"<163517202524.905629.18169486796771556053@Monstersaurus>","date":"2021-10-25T14:27:05","subject":"Re: [libcamera-devel] [PATCH v6 1/7] android: camera_stream:\n\tReplace post-processor nullptr check","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-10-25 05:53:53)\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Sat, Oct 23, 2021 at 04:02:56PM +0530, Umang Jain wrote:\n> > Instead of checking postProcessor for nullptr, replace this\n> > check with an assertion that checks if the camera stream's\n> > type is not Type::Direct. Since it makes no sense to call\n> > CameraStream::process() on a Type::Direct camera stream.\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_stream.cpp | 3 +--\n> > 1 file changed, 1 insertion(+), 2 deletions(-)\n> > \n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index f44a2717..ca25c4db 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -163,8 +163,7 @@ int CameraStream::process(const FrameBuffer &source,\n> > dest.fence = -1;\n> > }\n> > \n> > - if (!postProcessor_)\n> > - return 0;\n> > + ASSERT(type_ != Type::Direct);\n> \n> I wonder if this is really equivalent. It depends what the goal of the\n> check was. I've understood is as protecting against configure() not\n> having been called successfully, but that's just my interpretation, it\n> could have been there to protect against process() being called on\n> direct streams. As both are equally unlikely, I'm fine with the change.\n> Could you however move this to the beginning of the function ?\n> \n> Reviewed-by: Laurent Pinchart \n\nI would hazard a guess to think that the \"if (!postProcessor)\" was old\ncode that has followed refactoring that used to return early before\nprocessing JPEG streams (this line in git-blame has derived from when we\nhad a JPEG encoder wired directly in the camera_device I think.\n\nI think the assertion still makes sense though.\n\nReviewed-by: Kieran Bingham \n\n> \n> > \n> > /*\n> > * \\todo Buffer mapping and processing should be moved to a\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 30DA3BF415\n\tfor ;\n\tMon, 25 Oct 2021 14:27:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 884406486E;\n\tMon, 25 Oct 2021 16:27: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 8BBC460124\n\tfor ;\n\tMon, 25 Oct 2021 16:27:08 +0200 (CEST)","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 18A02E0A;\n\tMon, 25 Oct 2021 16:27:08 +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=\"cINXxG+M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635172028;\n\tbh=QlEUqhwhCTBZowJUMWkyVilIpfxqiVfk7IH51PhnZQY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=cINXxG+Myw/Op1IRetaXt0dVqaD2A+KO5uhwKPV7EijPYD753vyAjQAJFsZXVL0HJ\n\ta/PCe/LT9R6V+YiDJdksFFg8zk9zXCLlj7VZWIcuAIxc4ecQbTza0cn1MZEd1XW9cH\n\t6QCvWBEzukjuDekyHnn66EhhNuF/P6JuyPEgZFCU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-2-umang.jain@ideasonboard.com>\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tUmang Jain ","Date":"Mon, 25 Oct 2021 15:27:05 +0100","Message-ID":"<163517202524.905629.18169486796771556053@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v6 1/7] android: camera_stream:\n\tReplace post-processor nullptr check","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\" "}}]