[{"id":20405,"web_url":"https://patchwork.libcamera.org/comment/20405/","msgid":"","date":"2021-10-25T04:58:03","subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ 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:57PM +0530, Umang Jain wrote:\n> Instead of simply returning if encoder_ is nullptr, fail hard\n> via an assertion. It is quite unlikely that encoder_ will be\n> nullptr in PostProcessorJpeg::process() so, be loud about\n> the failure.\n\nIf it was only quite unlikely an assertion wouldn't be acceptable. It\nhas to be impossible. That's supposed to be the case, so the change is\nfine, but I'd write the commit message as \"encoder_ could only be null\nas a result of a fatal bug in the code, so ...\".\n\nReviewed-by: Laurent Pinchart \n\n> Signed-off-by: Umang Jain \n> ---\n> src/android/jpeg/post_processor_jpeg.cpp | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n> \n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 699576ef..49483836 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -102,9 +102,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t\t\t CameraBuffer *destination,\n> \t\t\t Camera3RequestDescriptor *request)\n> {\n> -\tif (!encoder_)\n> -\t\treturn 0;\n> -\n> +\tASSERT(encoder_);\n> \tASSERT(destination->numPlanes() == 1);\n> \n> \tconst CameraMetadata &requestMetadata = request->settings_;","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 C1778BF415\n\tfor ;\n\tMon, 25 Oct 2021 04:58:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CAD664870;\n\tMon, 25 Oct 2021 06:58: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 EC5B160237\n\tfor ;\n\tMon, 25 Oct 2021 06:58:24 +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 59D39E0A;\n\tMon, 25 Oct 2021 06:58:24 +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=\"Gt09UpXr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635137904;\n\tbh=fpa/bZIpdxBD+xNcFprErxeVp3ZVWuCiLhut7gR/CCM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gt09UpXrLZHkEL3UyvP9sewHixVd7G5+B++agL/aKjYkrJ/FXsb4deexCH+yCXVSs\n\tbLxiAu0RpeCIs7plwLw1eGelYMWA1PybIPzPLGaV2Md+2KxDmDXBKbTE+FtlnZmvCy\n\tv7FS9TrVc8waOuL9yqKTN1qo5jb9OeQC9RRKV9FI=","Date":"Mon, 25 Oct 2021 07:58:03 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211023103302.152769-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ 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":20408,"web_url":"https://patchwork.libcamera.org/comment/20408/","msgid":"","date":"2021-10-25T05:18:29","subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ 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:58 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:57PM +0530, Umang Jain wrote:\n> > Instead of simply returning if encoder_ is nullptr, fail hard\n> > via an assertion. It is quite unlikely that encoder_ will be\n> > nullptr in PostProcessorJpeg::process() so, be loud about\n> > the failure.\n>\n> If it was only quite unlikely an assertion wouldn't be acceptable. It\n> has to be impossible. That's supposed to be the case, so the change is\n> fine, but I'd write the commit message as \"encoder_ could only be null\n> as a result of a fatal bug in the code, so ...\".\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nI think this causes a crash of a process calling libcamera.\nIs it acceptable?\nI would like to know the rule of using ASSERT against returning error\nin libcamera.\n\n-Hiro\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/jpeg/post_processor_jpeg.cpp | 4 +---\n> > 1 file changed, 1 insertion(+), 3 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 699576ef..49483836 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -102,9 +102,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > CameraBuffer *destination,\n> > Camera3RequestDescriptor *request)\n> > {\n> > - if (!encoder_)\n> > - return 0;\n> > -\n> > + ASSERT(encoder_);\n> > ASSERT(destination->numPlanes() == 1);\n> >\n> > const CameraMetadata &requestMetadata = request->settings_;\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 64191BF415\n\tfor ;\n\tMon, 25 Oct 2021 05:18:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDC0660237;\n\tMon, 25 Oct 2021 07:18:42 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A6CF60237\n\tfor ;\n\tMon, 25 Oct 2021 07:18:41 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id g10so13108072edj.1\n\tfor ;\n\tSun, 24 Oct 2021 22:18:41 -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=\"CA0F+vh9\"; 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=PXju7kAubinOooikbm5c0sK6hyd4MIX2PkVUkR0u5S0=;\n\tb=CA0F+vh9DMhFntm5Dh7GOxi06YAsj8aqykzyVe7LpdXZeu9UwX3XGOehjPDQWsg3Cq\n\t+z4XrFRfy+afpn1S19ryjy1GuclQ2ay5WxxhCMiFQkO6rnI2nDTnwqaa8ov2+F+YWhDs\n\tKO44rd0i1mmu7PL64bCmee6V8/saRTXWMPvKk=","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=PXju7kAubinOooikbm5c0sK6hyd4MIX2PkVUkR0u5S0=;\n\tb=OxkTa7q2B9tDCanDZpDX/Xvquadr1CbEs463KNsUwT5hhSDpmj1c5boTxYWaISQ/3M\n\tagCXNfl+UxP7EjueXUZv/Rpo56/2dRrM9h+tWBBGpIgONHIKScMogb9NTqr1E7uyXbn3\n\tfrB4nrCmw5EKYnFB+61NGGLSs/OdgURQ5cdBh7gfLVrU4q/3nA/bTaJztP+W2Nv6EGIl\n\t36SMG8ySw3/6XXi4p/icGYsSjjHISu3yVq3XY+lO4PvSI59XTYwkYopf3Hf3h+39wpqD\n\tqQDgkB/kyuT+dqZXtv7bjZ4YPnguS3ID/iuPJGQUquB37XiPXs6eOLbYEuyVXZk3og2e\n\tfHww==","X-Gm-Message-State":"AOAM530iffxC/GYCHOnwpA+AuHCoUFqNewdNscGbarD8s/FSUqx1KoyV\n\tLBPv2SKiurfgxfnKA3H23ogFj9hTkcVeUYnTHson/jYf83A=","X-Google-Smtp-Source":"ABdhPJzDihFrxar3+BMNS9jDtvEnz8iCAmNibe9u+4sSR809AToGmnInbV12CaDNTRP30wwr2NcD4IOzjVPXGVVzo/Y=","X-Received":"by 2002:a05:6402:26d3:: with SMTP id\n\tx19mr23621265edd.291.1635139120960; \n\tSun, 24 Oct 2021 22:18:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-3-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 25 Oct 2021 14:18:29 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ 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":20410,"web_url":"https://patchwork.libcamera.org/comment/20410/","msgid":"","date":"2021-10-25T05:37:51","subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ nullptr check","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Oct 25, 2021 at 02:18:29PM +0900, Hirokazu Honda wrote:\n> On Mon, Oct 25, 2021 at 1:58 PM Laurent Pinchart wrote:\n> > On Sat, Oct 23, 2021 at 04:02:57PM +0530, Umang Jain wrote:\n> > > Instead of simply returning if encoder_ is nullptr, fail hard\n> > > via an assertion. It is quite unlikely that encoder_ will be\n> > > nullptr in PostProcessorJpeg::process() so, be loud about\n> > > the failure.\n> >\n> > If it was only quite unlikely an assertion wouldn't be acceptable. It\n> > has to be impossible. That's supposed to be the case, so the change is\n> > fine, but I'd write the commit message as \"encoder_ could only be null\n> > as a result of a fatal bug in the code, so ...\".\n> >\n> > Reviewed-by: Laurent Pinchart \n> \n> I think this causes a crash of a process calling libcamera.\n> Is it acceptable?\n> I would like to know the rule of using ASSERT against returning error\n> in libcamera.\n\nI don't think we have documented rules (and that should be fixed). My\npersonal preference at the moment is to use assertions to check for\nconditions that are supposed to never happen, and indicate a clear bug\nsomewhere that we can't recover from. In this case, for instance,\nencoder_ can only be null if configure() hasn't been called\nsuccessfully, which would mean that the CameraDevice implementation has\na fundamental bug. We can't recover from that, as we don't know what\nother problems that bug could cause.\n\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/android/jpeg/post_processor_jpeg.cpp | 4 +---\n> > > 1 file changed, 1 insertion(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > index 699576ef..49483836 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > @@ -102,9 +102,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > CameraBuffer *destination,\n> > > Camera3RequestDescriptor *request)\n> > > {\n> > > - if (!encoder_)\n> > > - return 0;\n> > > -\n> > > + ASSERT(encoder_);\n> > > ASSERT(destination->numPlanes() == 1);\n> > >\n> > > const CameraMetadata &requestMetadata = request->settings_;","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 80F4CBF415\n\tfor ;\n\tMon, 25 Oct 2021 05:38:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E180164870;\n\tMon, 25 Oct 2021 07:38:14 +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 ED6EA60237\n\tfor ;\n\tMon, 25 Oct 2021 07:38:12 +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 59B10E0A;\n\tMon, 25 Oct 2021 07:38:12 +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=\"m8XTN2By\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635140292;\n\tbh=FbGTktQW7gXXa7+JN/YBUTsvCrUnO10DFrYeWvzEo84=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m8XTN2Byg849nIEA2Osh2KeO0f2VgxW/h0WpYSE3+A/ibCLh2aVvUxlvcr5OkHw1O\n\t31RKuRx9KnCf9OBq9VJjBvXLlbopUue4DAr7GCwIgY0PMtaF+xkkP0IJ0hOQFrTcFD\n\tAE+HiTzD9imFpcl9+e4a86SidJTsJ873Iyn21yk4=","Date":"Mon, 25 Oct 2021 08:37:51 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-3-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ 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":20412,"web_url":"https://patchwork.libcamera.org/comment/20412/","msgid":"","date":"2021-10-25T05:43:05","subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ nullptr check","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Oct 25, 2021 at 2:38 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Oct 25, 2021 at 02:18:29PM +0900, Hirokazu Honda wrote:\n> > On Mon, Oct 25, 2021 at 1:58 PM Laurent Pinchart wrote:\n> > > On Sat, Oct 23, 2021 at 04:02:57PM +0530, Umang Jain wrote:\n> > > > Instead of simply returning if encoder_ is nullptr, fail hard\n> > > > via an assertion. It is quite unlikely that encoder_ will be\n> > > > nullptr in PostProcessorJpeg::process() so, be loud about\n> > > > the failure.\n> > >\n> > > If it was only quite unlikely an assertion wouldn't be acceptable. It\n> > > has to be impossible. That's supposed to be the case, so the change is\n> > > fine, but I'd write the commit message as \"encoder_ could only be null\n> > > as a result of a fatal bug in the code, so ...\".\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> >\n> > I think this causes a crash of a process calling libcamera.\n> > Is it acceptable?\n> > I would like to know the rule of using ASSERT against returning error\n> > in libcamera.\n>\n> I don't think we have documented rules (and that should be fixed). My\n> personal preference at the moment is to use assertions to check for\n> conditions that are supposed to never happen, and indicate a clear bug\n> somewhere that we can't recover from. In this case, for instance,\n> encoder_ can only be null if configure() hasn't been called\n> successfully, which would mean that the CameraDevice implementation has\n> a fundamental bug. We can't recover from that, as we don't know what\n> other problems that bug could cause.\n>\n\nI agree to the rule. May I ask you to document somewhere?\n\nReviewed-by: Hirokazu Honda \n\n> > > > Signed-off-by: Umang Jain \n> > > > ---\n> > > > src/android/jpeg/post_processor_jpeg.cpp | 4 +---\n> > > > 1 file changed, 1 insertion(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > index 699576ef..49483836 100644\n> > > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > @@ -102,9 +102,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > > CameraBuffer *destination,\n> > > > Camera3RequestDescriptor *request)\n> > > > {\n> > > > - if (!encoder_)\n> > > > - return 0;\n> > > > -\n> > > > + ASSERT(encoder_);\n> > > > ASSERT(destination->numPlanes() == 1);\n> > > >\n> > > > const CameraMetadata &requestMetadata = request->settings_;\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 05E3DBDB1C\n\tfor ;\n\tMon, 25 Oct 2021 05:43:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79E3B6486E;\n\tMon, 25 Oct 2021 07:43:19 +0200 (CEST)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F6E560237\n\tfor ;\n\tMon, 25 Oct 2021 07:43:17 +0200 (CEST)","by mail-ed1-x530.google.com with SMTP id j10so6851375eds.12\n\tfor ;\n\tSun, 24 Oct 2021 22:43:17 -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=\"SLSUazhG\"; 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=1jfNHwkClsiAVG1QpWtzVFicbn99YRmEKsbCKj4LVKg=;\n\tb=SLSUazhGgpFsDQn6htayyKdrkj9mK97ii+VNdXWOM6eH2467q7m2j0VeEYcOOcnJ/r\n\tkc/EwzRNyLCgPXq0eorZ5QNz8PJfNzEMk/Gt4uM1kxNvjtInNIqwtrmkCo8hbYg9ZDsP\n\tsNcvCzCAqd0Y2McJx53WZ/vrx5Ge7oJCtzwWo=","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=1jfNHwkClsiAVG1QpWtzVFicbn99YRmEKsbCKj4LVKg=;\n\tb=xHtn0CKrArtoQH//zQXzNXbcgQ0pq/P9nmRg9616EPYJ+ryIJzuzwyKtCsUwTRyE6u\n\tqjEqz4t5SM5qp0G3O6FEQUr4jmnYyB4wnuaOvHkLpCCtlYGLz/6VaLvaMnNgBJ+bT3Dj\n\tfBvOoIPsmRrjFA9PgcmEOoLYWf0WXKG1qTeoiXB7i3GDl8AkMwRcys+cVX2TC1i1hUGr\n\t1XIRkSwiTVVidLtl4/xwBmy8lGKt6kmNAQUFBfXQvDj4s1ha4jvQ3eNo/EI4mzdpbIsG\n\tz4Zhr3brKJsd8EuwChB6MMD2+xUfPfotrBjivRPclXrVcd5p6HY0Xiis3mEWbdqgA/vN\n\txdug==","X-Gm-Message-State":"AOAM531/r84NMYsR+P9Xsj5v4NxQT+WafpgDXJ9Lo3+vHqcz5Y1OtOCT\n\tCZ+qo40uQ3U0j78C21b6U9lp4AZOEDiV9DzYhcHITw==","X-Google-Smtp-Source":"ABdhPJwr7Ompb7AVJ+MmTyiXwbXaNtnmYCo94OxaKtw2JEAMd0CuF4LaZe2uGNTjChXdBZCvcvRGq0xkCAjEB6czVeo=","X-Received":"by 2002:a05:6402:3554:: with SMTP id\n\tf20mr23554394edd.354.1635140597138; \n\tSun, 24 Oct 2021 22:43:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-3-umang.jain@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 25 Oct 2021 14:43:05 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ 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":20445,"web_url":"https://patchwork.libcamera.org/comment/20445/","msgid":"<163517288177.905629.5202618843000270678@Monstersaurus>","date":"2021-10-25T14:41:21","subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ nullptr check","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hirokazu Honda (2021-10-25 06:43:05)\n> Hi Laurent,\n> \n> On Mon, Oct 25, 2021 at 2:38 PM Laurent Pinchart\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Mon, Oct 25, 2021 at 02:18:29PM +0900, Hirokazu Honda wrote:\n> > > On Mon, Oct 25, 2021 at 1:58 PM Laurent Pinchart wrote:\n> > > > On Sat, Oct 23, 2021 at 04:02:57PM +0530, Umang Jain wrote:\n> > > > > Instead of simply returning if encoder_ is nullptr, fail hard\n> > > > > via an assertion. It is quite unlikely that encoder_ will be\n> > > > > nullptr in PostProcessorJpeg::process() so, be loud about\n> > > > > the failure.\n> > > >\n> > > > If it was only quite unlikely an assertion wouldn't be acceptable. It\n> > > > has to be impossible. That's supposed to be the case, so the change is\n> > > > fine, but I'd write the commit message as \"encoder_ could only be null\n> > > > as a result of a fatal bug in the code, so ...\".\n> > > >\n> > > > Reviewed-by: Laurent Pinchart \n> > >\n> > > I think this causes a crash of a process calling libcamera.\n> > > Is it acceptable?\n> > > I would like to know the rule of using ASSERT against returning error\n> > > in libcamera.\n> >\n> > I don't think we have documented rules (and that should be fixed). My\n> > personal preference at the moment is to use assertions to check for\n> > conditions that are supposed to never happen, and indicate a clear bug\n> > somewhere that we can't recover from. In this case, for instance,\n> > encoder_ can only be null if configure() hasn't been called\n> > successfully, which would mean that the CameraDevice implementation has\n> > a fundamental bug. We can't recover from that, as we don't know what\n> > other problems that bug could cause.\n> >\n\nYes, ASSERT means that something has happened which is supposed to be\nimpossible, and I usually expect it to be used in the case where it\nmight be possible that a developer could change something in the future,\nor incorrectly use an object... such that the ASSERT would fire and\ncatch the attention of the developer ... and isn't something we'd expect\nend users to ever hit.\n\n> I agree to the rule. May I ask you to document somewhere?\n\nI guess this would go in the coding style somewhere? Maybe I assumed\nthat my interpretation of assertions was generic to 'all' projects?\n\nDo other projects vary wildly in their expectations of using assertions?\n\nanyway, for this patch.\n\n\nReviewed-by: Kieran Bingham \n\n> \n> Reviewed-by: Hirokazu Honda \n> \n> > > > > Signed-off-by: Umang Jain \n> > > > > ---\n> > > > > src/android/jpeg/post_processor_jpeg.cpp | 4 +---\n> > > > > 1 file changed, 1 insertion(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > index 699576ef..49483836 100644\n> > > > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > > > @@ -102,9 +102,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > > > > CameraBuffer *destination,\n> > > > > Camera3RequestDescriptor *request)\n> > > > > {\n> > > > > - if (!encoder_)\n> > > > > - return 0;\n> > > > > -\n> > > > > + ASSERT(encoder_);\n> > > > > ASSERT(destination->numPlanes() == 1);\n> > > > >\n> > > > > const CameraMetadata &requestMetadata = request->settings_;\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 B3858BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 14:41:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 09AAC6486E;\n\tMon, 25 Oct 2021 16:41:26 +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 C13DA60124\n\tfor ;\n\tMon, 25 Oct 2021 16:41:24 +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 5287CE0A;\n\tMon, 25 Oct 2021 16:41:24 +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=\"HD5t1kSn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635172884;\n\tbh=j3YjOUiXvI43cd4jkglxtndq8Ob5bmVhS5naUBghDW8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=HD5t1kSntqBNMJZfwa1cBIPcXvqklA/4cHKEtUKR6aMhcpip5uy2JGdMS6bs9fNNd\n\tnpNpVk0O46MQ81k49N9uFSLd2bBEld6BWN8XukBW+tnHMajCuA9KCPPy6kvT6f6k2O\n\tpqF7fEOKfiaaJW7L/A9LpMXnYKBiZvgXDnt2BDuc=","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-3-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t","From":"Kieran Bingham ","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","Date":"Mon, 25 Oct 2021 15:41:21 +0100","Message-ID":"<163517288177.905629.5202618843000270678@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v6 2/7] android: post_processor_jpeg:\n\tReplace encoder_ 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\" "}}]