[{"id":32548,"web_url":"https://patchwork.libcamera.org/comment/32548/","msgid":"<6i6a56jpiju7dawyypd4az64xryck73b6pu5kjck3an7zcsygy@kekkjw2yb53x>","date":"2024-12-05T16:39:52","subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote:\n> According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used\n> for requests that have not done any processing. When a request is\n> completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead.\n>\n> To avoid code duplication, when CameraMetadata cannot be generated,\n> CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result.\n>\n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n> ---\n> src/android/camera_device.cpp | 17 ++++++++++++-----\n> 1 file changed, 12 insertions(+), 5 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 497d363d6..dd2c603e0 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request)\n\nYou should update the comment before the if()\n\n> \t\t\t\t<< \" not successfully completed: \"\n> \t\t\t\t<< request->status();\n>\n> -\t\tabortRequest(descriptor);\n> -\t\tcompleteDescriptor(descriptor);\n> -\n> -\t\treturn;\n\nIs it ok to continue and try to fetch metadata from a Request in error\nstatus ?\n\nAlso, the\n\n\tif (request->status() != Request::RequestComplete) {\n\ncase, doesn't fall in\n\n/**\n * S6. Error management:\n *\n\n ...\n\n * - The failure of an entire capture to occur must be reported by the HAL by\n * calling notify() with ERROR_REQUEST. Individual errors for the result\n * metadata or the output buffers must not be reported in this case.\n\nI think the\n\n\t\tabortRequest(descriptor);\n\t\tcompleteDescriptor(descriptor);\n\nsequence should be kept here ?\n\nSpecifically, if you don't call completeDescriptor() the descriptor\nwill remain in pending state and stall the queue of complete\ndescriptors\n\n\n> +\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> \t}\n>\n> \t/*\n> @@ -1239,7 +1236,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t */\n> \tdescriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> \tif (!descriptor->resultMetadata_) {\n> -\t\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> +\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n\nTo be honest I don't see what we gain by delaying the notification\n\n\n>\n> \t\t/*\n> \t\t * The camera framework expects an empty metadata pack on error.\n> @@ -1325,6 +1322,16 @@ void CameraDevice::sendCaptureResults()\n> \t\tdescriptors_.pop();\n>\n> \t\tsendCaptureResult(descriptor.get());\n> +\n> +\t\t/*\n> +\t\t * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some\n> +\t\t * of the expected result metadata might not be available\n> +\t\t * because the capture is cancelled by the camera. Only notify\n> +\t\t * it when the final result is sent, since Android will ignore\n> +\t\t * the following metadata.\n> +\t\t */\n> +\t\tif (descriptor->status_ == Camera3RequestDescriptor::Status::Error)\n> +\t\t\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n\nIf my understanding of the specs if correct, I think in case of\n\n\tif (request->status() != Request::RequestComplete) {\n\nthe right error code to notify is ERROR_REQUEST, so if my\nunderstanding is correct, I would rather drop this whole change.\n\nOr am I mistaken maybe ? Have you got any failure in CTS or other\ntests because of this ?\n\n> \t}\n> }\n>\n> --\n> 2.47.0.338.g60cca15819-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6780BBDB1C\n\tfor ;\n\tThu, 5 Dec 2024 16:39:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 194D76610E;\n\tThu, 5 Dec 2024 17:39:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E96D266107\n\tfor ;\n\tThu, 5 Dec 2024 17:39:55 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 06F227E2;\n\tThu, 5 Dec 2024 17:39:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jUVOcP6t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733416767;\n\tbh=I4/SSt5+RT+HYwnZA3uXg9OpxMloL1vBKfa5OUbHe0w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jUVOcP6tUbOspySWQmo7dKmLy9ToFlp/JBXXCxcmTdyoYbO4ma4MHxLVHZlnYZFXE\n\tNTCjWFK2hlaYl2rjQxWrq6MtUD2VW0u1VjHQPQ5lWI7m90cF1sxrVEaMgn0IdrLiqy\n\t3RrbpfZuhslFk8HBjghN9VTMvZYvD/biW4sKpong=","Date":"Thu, 5 Dec 2024 17:39:52 +0100","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","Message-ID":"<6i6a56jpiju7dawyypd4az64xryck73b6pu5kjck3an7zcsygy@kekkjw2yb53x>","References":"<20241204164137.3938891-1-chenghaoyang@chromium.org>\n\t<20241204164137.3938891-6-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241204164137.3938891-6-chenghaoyang@chromium.org>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32623,"web_url":"https://patchwork.libcamera.org/comment/32623/","msgid":"","date":"2024-12-09T10:03:48","subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote:\n> > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used\n> > for requests that have not done any processing. When a request is\n> > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead.\n> >\n> > To avoid code duplication, when CameraMetadata cannot be generated,\n> > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result.\n> >\n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/android/camera_device.cpp | 17 ++++++++++++-----\n> > 1 file changed, 12 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 497d363d6..dd2c603e0 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request)\n>\n> You should update the comment before the if()\n\nUpdated. Please take another look.\n\n>\n> > << \" not successfully completed: \"\n> > << request->status();\n> >\n> > - abortRequest(descriptor);\n> > - completeDescriptor(descriptor);\n> > -\n> > - return;\n>\n> Is it ok to continue and try to fetch metadata from a Request in error\n> status ?\n\nI think so, as libcamera core library sets a Request in error status\nif and only if there's a buffer in error status. There might be some\nvalid metadata in the Request.\n\n>\n> Also, the\n>\n> if (request->status() != Request::RequestComplete) {\n>\n> case, doesn't fall in\n>\n> /**\n> * S6. Error management:\n> *\n>\n> ...\n>\n> * - The failure of an entire capture to occur must be reported by the HAL by\n> * calling notify() with ERROR_REQUEST. Individual errors for the result\n> * metadata or the output buffers must not be reported in this case.\n>\n> I think the\n>\n> abortRequest(descriptor);\n> completeDescriptor(descriptor);\n>\n> sequence should be kept here ?\n\nNot really. Perhaps we should update `camera3.h`, while this is what I\nwanted to discuss with you in the previous version of this patch:\n\nCAMERA3_MSG_ERROR_REQUEST is mostly used for requests that haven't\ndone any processing.\nCAMERA3_MSG_ERROR_RESULT is used for requests that have missing metadata.\n\nA request in error status doesn't fall into either of the cases.\n\nIf you can take another look at my last comments in `[PATCH v2 6/9]\nandroid: Cleanup CAMERA3_MSG_ERROR_REQUEST`, we can align and discuss\nhow to proceed here. Thanks :)\n\nRef: https://source.android.com/reference/hal/structcamera3__device__ops\n\n>\n> Specifically, if you don't call completeDescriptor() the descriptor\n> will remain in pending state and stall the queue of complete\n> descriptors\n\nAs we don't return in this if clause, either there is at least one\nbuffer to be post-processed, or completeDescriptor() will be called on\nthe descriptor. Let me know if I misunderstand anything.\n\n>\n>\n> > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > }\n> >\n> > /*\n> > @@ -1239,7 +1236,7 @@ void CameraDevice::requestComplete(Request *request)\n> > */\n> > descriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> > if (!descriptor->resultMetadata_) {\n> > - notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>\n> To be honest I don't see what we gain by delaying the notification\n\nWe can also remove the logic entirely: getResultMetadata() only fails\nto return a valid CameraMetadata with memory issues I think, not due\nto some corrupted Request::metadata(). It will be removed in the end\nanyways.\n\nWDYT?\n\n>\n>\n> >\n> > /*\n> > * The camera framework expects an empty metadata pack on error.\n> > @@ -1325,6 +1322,16 @@ void CameraDevice::sendCaptureResults()\n> > descriptors_.pop();\n> >\n> > sendCaptureResult(descriptor.get());\n> > +\n> > + /*\n> > + * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some\n> > + * of the expected result metadata might not be available\n> > + * because the capture is cancelled by the camera. Only notify\n> > + * it when the final result is sent, since Android will ignore\n> > + * the following metadata.\n> > + */\n> > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)\n> > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n>\n> If my understanding of the specs if correct, I think in case of\n>\n> if (request->status() != Request::RequestComplete) {\n>\n> the right error code to notify is ERROR_REQUEST, so if my\n> understanding is correct, I would rather drop this whole change.\n>\n> Or am I mistaken maybe ? Have you got any failure in CTS or other\n> tests because of this ?\n\nYeah that's what I wanted to fix with this patch:\n```\n3.4 For captures that will produce some results, the HAL must not call\nCAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure.\n```\n\nPreviously, we just ignored any potentially available metadata and\nbuffers and notified CAMERA3_MSG_ERROR_REQUEST on a request in error\nstatus. This doesn't seem correct to me, while I believe it still fits\nin the Android API.\n\nRegarding failure in CTS or other tests, I just tried on mtkisp7.\nWithout this patch, CTS still passes. I think it should be fine\nwithout this patch, if it's controversial.\n\nBR,\nHarvey\n\n>\n> > }\n> > }\n> >\n> > --\n> > 2.47.0.338.g60cca15819-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B5D5EBD80A\n\tfor ;\n\tMon, 9 Dec 2024 10:04:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90D7E67E55;\n\tMon, 9 Dec 2024 11:04:01 +0100 (CET)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C3CAF618AE\n\tfor ;\n\tMon, 9 Dec 2024 11:03:59 +0100 (CET)","by mail-lj1-x232.google.com with SMTP id\n\t38308e7fff4ca-2ffa8df8850so43601891fa.3\n\tfor ;\n\tMon, 09 Dec 2024 02:03:59 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"UTJcD3Vb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733738639; x=1734343439;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=vIMui6jzEGswmpXqW7XGe38V0ooCJ7Frx2/HUgsMCIs=;\n\tb=UTJcD3VbLOGoxiATXVkdXEDIVnhGn9lz0ORABOj04tn6q+Pi4bGgd/vXIVCIhOhkn7\n\tSG+eOYSEEez+EPjLUvkQZWQKDE9GhP0/KL5ldDgEjPrZaTWrgLxl4/XGuzm9wi6Y73Jz\n\tdsu51jvnXGhvm6iuonp32m9K2JUTSg7xc1Raw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733738639; x=1734343439;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=vIMui6jzEGswmpXqW7XGe38V0ooCJ7Frx2/HUgsMCIs=;\n\tb=amVPyEyuH3mVZMhisv6nn4aeFBClEtIlHEsklE4Kyq5VAaakuOvuxzNlNTC1ENIGH6\n\t+R1VTVwfzrF6YmVCooSPYKH0DUzo4XJ4l4P5OuHyP0OzSyVUiuAmfaaEunYgouGh1J+y\n\taD8+muQFG/o2fFeOdF9JLBm/w56E0nTMuz5IMdIJ3mI2I4+g+mpy5kB/epL2mobI8eok\n\tjGCeYdyIn0F1JPqHTOjU66IfQDWnFzfG3Nbjy+X3m5BGSe5/issNEQAmzAB/47aRZCQ6\n\tvyq4jD0KFHEctdnAEADvmPb7wAlylLlQQ4t0STM3/cH7aM/aDXdtN22F9bMd1raYk7es\n\tl2+A==","X-Gm-Message-State":"AOJu0YzrsnyOQ5dCG4B7M/6Sd40dpu6s7YesGIlOKq8EHSv+Q0quae4s\n\tc2/dzoiVpat7pQSCuEp8DznbknCUJZnTi5IB7I9wFLzddpHi2aqyXRaY6oKlKDRg+tDEbVhjf5L\n\tQxf26p/KewF4cCISEwt7/ofxNxqLVf/6EvAhx","X-Gm-Gg":"ASbGncuXpbHisx/bd8KZKspsPFx9J7CUr8kvgItLf60Xvauu7JHZOGiosXFjNYzfQHJ\n\tGV7YmY4XiY5USbPyyA50k2OobwrMKIcCxQZNCmPeEx4Q1d7KcqrMKe7BvqfQ=","X-Google-Smtp-Source":"AGHT+IHvaMnYBz8SGo+lqaRubSEQflkycGfoU4NkhMeJrENpEoNACjgbch76RPagJb7EJW7LqkR9Qxn/uSchipbqzto=","X-Received":"by 2002:a2e:9082:0:b0:300:322e:6a with SMTP id\n\t38308e7fff4ca-300322e020emr39896251fa.23.1733738638837; \n\tMon, 09 Dec 2024 02:03:58 -0800 (PST)","MIME-Version":"1.0","References":"<20241204164137.3938891-1-chenghaoyang@chromium.org>\n\t<20241204164137.3938891-6-chenghaoyang@chromium.org>\n\t<6i6a56jpiju7dawyypd4az64xryck73b6pu5kjck3an7zcsygy@kekkjw2yb53x>","In-Reply-To":"<6i6a56jpiju7dawyypd4az64xryck73b6pu5kjck3an7zcsygy@kekkjw2yb53x>","From":"Cheng-Hao Yang ","Date":"Mon, 9 Dec 2024 18:03:48 +0800","Message-ID":"","Subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32627,"web_url":"https://patchwork.libcamera.org/comment/32627/","msgid":"","date":"2024-12-09T10:27:13","subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Mon, Dec 9, 2024 at 6:03 PM Cheng-Hao Yang wrote:\n>\n> Hi Jacopo,\n>\n> On Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi\n> wrote:\n> >\n> > Hi Harvey\n> >\n> > On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote:\n> > > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used\n> > > for requests that have not done any processing. When a request is\n> > > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead.\n> > >\n> > > To avoid code duplication, when CameraMetadata cannot be generated,\n> > > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result.\n> > >\n> > > Signed-off-by: Han-Lin Chen \n> > > Co-developed-by: Harvey Yang \n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > src/android/camera_device.cpp | 17 ++++++++++++-----\n> > > 1 file changed, 12 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 497d363d6..dd2c603e0 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request)\n> >\n> > You should update the comment before the if()\n>\n> Updated. Please take another look.\n>\n> >\n> > > << \" not successfully completed: \"\n> > > << request->status();\n> > >\n> > > - abortRequest(descriptor);\n> > > - completeDescriptor(descriptor);\n> > > -\n> > > - return;\n> >\n> > Is it ok to continue and try to fetch metadata from a Request in error\n> > status ?\n>\n> I think so, as libcamera core library sets a Request in error status\n> if and only if there's a buffer in error status. There might be some\n> valid metadata in the Request.\n>\n> >\n> > Also, the\n> >\n> > if (request->status() != Request::RequestComplete) {\n> >\n> > case, doesn't fall in\n> >\n> > /**\n> > * S6. Error management:\n> > *\n> >\n> > ...\n> >\n> > * - The failure of an entire capture to occur must be reported by the HAL by\n> > * calling notify() with ERROR_REQUEST. Individual errors for the result\n> > * metadata or the output buffers must not be reported in this case.\n> >\n> > I think the\n> >\n> > abortRequest(descriptor);\n> > completeDescriptor(descriptor);\n> >\n> > sequence should be kept here ?\n>\n> Not really. Perhaps we should update `camera3.h`, while this is what I\n> wanted to discuss with you in the previous version of this patch:\n>\n> CAMERA3_MSG_ERROR_REQUEST is mostly used for requests that haven't\n> done any processing.\n> CAMERA3_MSG_ERROR_RESULT is used for requests that have missing metadata.\n>\n> A request in error status doesn't fall into either of the cases.\n>\n> If you can take another look at my last comments in `[PATCH v2 6/9]\n> android: Cleanup CAMERA3_MSG_ERROR_REQUEST`, we can align and discuss\n> how to proceed here. Thanks :)\n>\n> Ref: https://source.android.com/reference/hal/structcamera3__device__ops\n>\n> >\n> > Specifically, if you don't call completeDescriptor() the descriptor\n> > will remain in pending state and stall the queue of complete\n> > descriptors\n>\n> As we don't return in this if clause, either there is at least one\n> buffer to be post-processed, or completeDescriptor() will be called on\n> the descriptor. Let me know if I misunderstand anything.\n>\n> >\n> >\n> > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > }\n> > >\n> > > /*\n> > > @@ -1239,7 +1236,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > */\n> > > descriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> > > if (!descriptor->resultMetadata_) {\n> > > - notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> >\n> > To be honest I don't see what we gain by delaying the notification\n>\n> We can also remove the logic entirely: getResultMetadata() only fails\n> to return a valid CameraMetadata with memory issues I think, not due\n> to some corrupted Request::metadata(). It will be removed in the end\n> anyways.\n>\n> WDYT?\n>\n> >\n> >\n> > >\n> > > /*\n> > > * The camera framework expects an empty metadata pack on error.\n> > > @@ -1325,6 +1322,16 @@ void CameraDevice::sendCaptureResults()\n> > > descriptors_.pop();\n> > >\n> > > sendCaptureResult(descriptor.get());\n> > > +\n> > > + /*\n> > > + * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some\n> > > + * of the expected result metadata might not be available\n> > > + * because the capture is cancelled by the camera. Only notify\n> > > + * it when the final result is sent, since Android will ignore\n> > > + * the following metadata.\n> > > + */\n> > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)\n> > > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> >\n> > If my understanding of the specs if correct, I think in case of\n> >\n> > if (request->status() != Request::RequestComplete) {\n> >\n> > the right error code to notify is ERROR_REQUEST, so if my\n> > understanding is correct, I would rather drop this whole change.\n> >\n> > Or am I mistaken maybe ? Have you got any failure in CTS or other\n> > tests because of this ?\n>\n> Yeah that's what I wanted to fix with this patch:\n> ```\n> 3.4 For captures that will produce some results, the HAL must not call\n> CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure.\n> ```\n>\n> Previously, we just ignored any potentially available metadata and\n> buffers and notified CAMERA3_MSG_ERROR_REQUEST on a request in error\n> status. This doesn't seem correct to me, while I believe it still fits\n> in the Android API.\n>\n> Regarding failure in CTS or other tests, I just tried on mtkisp7.\n> Without this patch, CTS still passes. I think it should be fine\n> without this patch, if it's controversial.\n\nSorry, I take it back. I believe mtkisp7 passed CTS because we didn't\nproduce any failure.\n\nIn Android:\n```\nFor captures that will produce some results, the HAL must not call\nCAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure.\n```\n\nWith partial results, it cannot be guaranteed that there was no buffer\n/ metadata completed and sent to the application before we receive the\nrequestCompleted signal. We have to deal with this issue before the\npartial result patches land.\n\nBR,\nHarvey\n\n>\n> BR,\n> Harvey\n>\n> >\n> > > }\n> > > }\n> > >\n> > > --\n> > > 2.47.0.338.g60cca15819-goog\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D0459BD80A\n\tfor ;\n\tMon, 9 Dec 2024 10:27:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71C9B67E5F;\n\tMon, 9 Dec 2024 11:27:26 +0100 (CET)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D056167E47\n\tfor ;\n\tMon, 9 Dec 2024 11:27:24 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id\n\t38308e7fff4ca-3022c6155edso1532961fa.2\n\tfor ;\n\tMon, 09 Dec 2024 02:27:24 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"HloTXt6+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733740044; x=1734344844;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=b1bB+XBQ8qQID2/xT5Rtum0+5sPukTFIK4dLYeCHbp4=;\n\tb=HloTXt6+GrvaNqmAvb+4MM5O4j/6x2EuXJVyldBg0PLoEGdV9fJMYB8mgf5ZDKEuc5\n\tIVo/HHDsg7pcnveyTTHhHCiLNMpSomEle7ecHXaxgRT4pk31TezoTqWBVjS2cUrqFepN\n\t5ISEZkXr5rQ+B7/JaJBzcjr5F9EIirskf0rN0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733740044; x=1734344844;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=b1bB+XBQ8qQID2/xT5Rtum0+5sPukTFIK4dLYeCHbp4=;\n\tb=VUe+ytsGxXDllMuiEN0vf9i235yWQSEHtaDx8/Y/QBTtBjyC79y8UT5ffNN+UL3lSv\n\t2evwQezxp8nVml0Ey749KsoNrDY/+G0Y1E/nICyiRLhiim2K4m16c5oJdEZm3oYMapzD\n\trqc9yiaO/+T+q6wsC7Hu0oSvGoqPZLINAVoNeu2QXMTMEljEIQ3TU0jwJMeZvefJi08m\n\t82XTWbush796D1se7Udten8udRpRGLuF0Exts9OJkKk9pbxiYYuprxHbjPztTezDwuUO\n\tj0wlyfvE4PULx7j9wh3+FsbZoWVOk+Fi+raNRL3rqn4QJKXFbt0A8ljctp39moZ1cAeS\n\to9KA==","X-Gm-Message-State":"AOJu0YyUYzap4DjH+544wU2F9K1cop9mDsij18FKzeuFgnl0RB1vwO4a\n\tZDJKH8IXWrEfQL4lbaYyUoJAKSiJPh4DzwomuStslqBH/vylEPPna7CH5ztmTvQPMJFGgfuMNP7\n\tKbNhGRIoSlfRwIHIqcRusT9rLpvtbOLX2JItX","X-Gm-Gg":"ASbGncsUFS/vQCwrm+1FfvUYCDXX9B/j7XEWRxlnOvlfNBPRWpJBJDDHTXYmeZQwERF\n\tbxFRwlMDVNg8LoZVUo4VDR5GiNEfDqfqnxsorC6o5WABG/NP+LHQPzun+BwY=","X-Google-Smtp-Source":"AGHT+IFT/KYtY5nrI6VdW5QanAjKZrPDTuxgT3j79t7oXlAldVnRYeAP/Jr3k/EG1IY45HksWFeYJlXv/8hWtHJ+kys=","X-Received":"by 2002:a2e:be11:0:b0:300:1f2c:e3d1 with SMTP id\n\t38308e7fff4ca-3002f933d2emr33802161fa.23.1733740044018;\n\tMon, 09 Dec 2024 02:27:24 -0800 (PST)","MIME-Version":"1.0","References":"<20241204164137.3938891-1-chenghaoyang@chromium.org>\n\t<20241204164137.3938891-6-chenghaoyang@chromium.org>\n\t<6i6a56jpiju7dawyypd4az64xryck73b6pu5kjck3an7zcsygy@kekkjw2yb53x>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Mon, 9 Dec 2024 18:27:13 +0800","Message-ID":"","Subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32649,"web_url":"https://patchwork.libcamera.org/comment/32649/","msgid":"<2wckutph6ccrlqr2czgwq5eelxnmyte7bo3ol2y7ocm6opxqjk@xhcubkyqtpoo>","date":"2024-12-10T08:29:28","subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Mon, Dec 09, 2024 at 06:27:13PM +0800, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> On Mon, Dec 9, 2024 at 6:03 PM Cheng-Hao Yang wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Harvey\n> > >\n> > > On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote:\n> > > > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used\n> > > > for requests that have not done any processing. When a request is\n> > > > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead.\n> > > >\n> > > > To avoid code duplication, when CameraMetadata cannot be generated,\n> > > > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result.\n> > > >\n> > > > Signed-off-by: Han-Lin Chen \n> > > > Co-developed-by: Harvey Yang \n> > > > Signed-off-by: Harvey Yang \n> > > > ---\n> > > > src/android/camera_device.cpp | 17 ++++++++++++-----\n> > > > 1 file changed, 12 insertions(+), 5 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 497d363d6..dd2c603e0 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request)\n> > >\n> > > You should update the comment before the if()\n> >\n> > Updated. Please take another look.\n> >\n> > >\n> > > > << \" not successfully completed: \"\n> > > > << request->status();\n> > > >\n> > > > - abortRequest(descriptor);\n> > > > - completeDescriptor(descriptor);\n> > > > -\n> > > > - return;\n> > >\n> > > Is it ok to continue and try to fetch metadata from a Request in error\n> > > status ?\n> >\n> > I think so, as libcamera core library sets a Request in error status\n> > if and only if there's a buffer in error status. There might be some\n> > valid metadata in the Request.\n> >\n\nI guess the major takeaway here is that the libcamera error\nnotification is really not sophisticated enough.\n\n> > >\n> > > Also, the\n> > >\n> > > if (request->status() != Request::RequestComplete) {\n> > >\n> > > case, doesn't fall in\n> > >\n> > > /**\n> > > * S6. Error management:\n> > > *\n> > >\n> > > ...\n> > >\n> > > * - The failure of an entire capture to occur must be reported by the HAL by\n> > > * calling notify() with ERROR_REQUEST. Individual errors for the result\n> > > * metadata or the output buffers must not be reported in this case.\n> > >\n> > > I think the\n> > >\n> > > abortRequest(descriptor);\n> > > completeDescriptor(descriptor);\n> > >\n> > > sequence should be kept here ?\n> >\n> > Not really. Perhaps we should update `camera3.h`, while this is what I\n> > wanted to discuss with you in the previous version of this patch:\n> >\n> > CAMERA3_MSG_ERROR_REQUEST is mostly used for requests that haven't\n> > done any processing.\n> > CAMERA3_MSG_ERROR_RESULT is used for requests that have missing metadata.\n> >\n> > A request in error status doesn't fall into either of the cases.\n> >\n\nRight\n\n> > If you can take another look at my last comments in `[PATCH v2 6/9]\n> > android: Cleanup CAMERA3_MSG_ERROR_REQUEST`, we can align and discuss\n> > how to proceed here. Thanks :)\n> >\n> > Ref: https://source.android.com/reference/hal/structcamera3__device__ops\n> >\n\nYes, sorry I missed that and jumped on this version.\n\nI see your point: The RequestCancelled state doesn't tell us nothing\nabout metadata, but only that some buffer has failed.\n\nHowever, this means some processing has been done, and you're right\nthis should be transmitted to Android with an ERROR_RESULT code\n\n> > >\n> > > Specifically, if you don't call completeDescriptor() the descriptor\n> > > will remain in pending state and stall the queue of complete\n> > > descriptors\n> >\n> > As we don't return in this if clause, either there is at least one\n> > buffer to be post-processed, or completeDescriptor() will be called on\n> > the descriptor. Let me know if I misunderstand anything.\n\nThis is the part that scary me a bit.\n\nWhen we try to post-process we don't check for the source buffer\nvalidity, am I wrong ? I would be fine with your approach, but if we\ncontinue processing the Request even if it's in Cancelled state, then\nany access to the buffers should be checked and handled correctly ?\n\n> >\n> > >\n> > >\n> > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > > }\n> > > >\n> > > > /*\n> > > > @@ -1239,7 +1236,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > > */\n> > > > descriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> > > > if (!descriptor->resultMetadata_) {\n> > > > - notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > >\n> > > To be honest I don't see what we gain by delaying the notification\n> >\n> > We can also remove the logic entirely: getResultMetadata() only fails\n> > to return a valid CameraMetadata with memory issues I think, not due\n> > to some corrupted Request::metadata(). It will be removed in the end\n\nWell, failure to reserve space is a metadata failure, isn't it ?\n\n> > anyways.\n> >\n> > WDYT?\n> >\n\nThing is, to post-process you need metadata, don't you ? Is it worth\ntrying to post-process without metadata ?\n\n> > >\n> > >\n> > > >\n> > > > /*\n> > > > * The camera framework expects an empty metadata pack on error.\n> > > > @@ -1325,6 +1322,16 @@ void CameraDevice::sendCaptureResults()\n> > > > descriptors_.pop();\n> > > >\n> > > > sendCaptureResult(descriptor.get());\n> > > > +\n> > > > + /*\n> > > > + * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some\n> > > > + * of the expected result metadata might not be available\n> > > > + * because the capture is cancelled by the camera. Only notify\n> > > > + * it when the final result is sent, since Android will ignore\n> > > > + * the following metadata.\n> > > > + */\n> > > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)\n> > > > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > >\n> > > If my understanding of the specs if correct, I think in case of\n> > >\n> > > if (request->status() != Request::RequestComplete) {\n> > >\n> > > the right error code to notify is ERROR_REQUEST, so if my\n> > > understanding is correct, I would rather drop this whole change.\n> > >\n> > > Or am I mistaken maybe ? Have you got any failure in CTS or other\n> > > tests because of this ?\n> >\n> > Yeah that's what I wanted to fix with this patch:\n> > ```\n> > 3.4 For captures that will produce some results, the HAL must not call\n> > CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure.\n> > ```\n> >\n> > Previously, we just ignored any potentially available metadata and\n> > buffers and notified CAMERA3_MSG_ERROR_REQUEST on a request in error\n> > status. This doesn't seem correct to me, while I believe it still fits\n> > in the Android API.\n\nOk, I'll take this as we don't have expressive enough failures in\nlibcamera. Error handling paths are particularly hard to test, that's\nwhy I'm so cautious here, before this patch we considered\nRequestCancelled == Complete Request Failure and we didn't deliver\nanything to the framework for this request.\n\nMoving forward some metadata might be notified to the framework before\nrequestComplete() so ERROR_REQUEST is not the right code anymore.\n\nI'm fine with this (now that I understand it) but please let me know\nwhat do you think about checking the source buffer status when doing\npost-processing.\n\n> >\n> > Regarding failure in CTS or other tests, I just tried on mtkisp7.\n> > Without this patch, CTS still passes. I think it should be fine\n> > without this patch, if it's controversial.\n>\n> Sorry, I take it back. I believe mtkisp7 passed CTS because we didn't\n> produce any failure.\n\nYes, failure path are particularly hard to test, I know...\n\n\n>\n> In Android:\n> ```\n> For captures that will produce some results, the HAL must not call\n> CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure.\n> ```\n>\n> With partial results, it cannot be guaranteed that there was no buffer\n> / metadata completed and sent to the application before we receive the\n> requestCompleted signal. We have to deal with this issue before the\n> partial result patches land.\n\nI agree. Thanks for explaining, let me know what you think about the\nlast missing item (checking src buffer state when post-processing)\n\nThanks\n j\n\n>\n> BR,\n> Harvey\n>\n> >\n> > BR,\n> > Harvey\n> >\n> > >\n> > > > }\n> > > > }\n> > > >\n> > > > --\n> > > > 2.47.0.338.g60cca15819-goog\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7717CBD80A\n\tfor ;\n\tTue, 10 Dec 2024 08:29:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B31267E7D;\n\tTue, 10 Dec 2024 09:29:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1726608B6\n\tfor ;\n\tTue, 10 Dec 2024 09:29:31 +0100 (CET)","from ideasonboard.com (mob-5-90-141-199.net.vodafone.it\n\t[5.90.141.199])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A893752;\n\tTue, 10 Dec 2024 09:28:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HR5dxP5+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733819339;\n\tbh=XioFl5cNqEHmJbUtB+YnJHzii8IK+97kWo6uzaurE90=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HR5dxP5+EV+PZPosOPE8tQ51PSinlLQAbpu2We/rvlCz91S1sU/J+sKhfVLdt2Q5W\n\tPa9vNVG/OndWF8LBcwVt1rHhUa/FKqs9//g7mbHk8HAkU+5PKGMV1yBdJA+FJsZzKw\n\t4CfqtFdU+/+EAUaPiiEJxpLRUn7h/BV/ZAhOEFVc=","Date":"Tue, 10 Dec 2024 09:29:28 +0100","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","Message-ID":"<2wckutph6ccrlqr2czgwq5eelxnmyte7bo3ol2y7ocm6opxqjk@xhcubkyqtpoo>","References":"<20241204164137.3938891-1-chenghaoyang@chromium.org>\n\t<20241204164137.3938891-6-chenghaoyang@chromium.org>\n\t<6i6a56jpiju7dawyypd4az64xryck73b6pu5kjck3an7zcsygy@kekkjw2yb53x>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32651,"web_url":"https://patchwork.libcamera.org/comment/32651/","msgid":"","date":"2024-12-10T09:18:51","subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, Dec 10, 2024 at 4:29 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Mon, Dec 09, 2024 at 06:27:13PM +0800, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, Dec 9, 2024 at 6:03 PM Cheng-Hao Yang wrote:\n> > >\n> > > Hi Jacopo,\n> > >\n> > > On Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi Harvey\n> > > >\n> > > > On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote:\n> > > > > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used\n> > > > > for requests that have not done any processing. When a request is\n> > > > > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead.\n> > > > >\n> > > > > To avoid code duplication, when CameraMetadata cannot be generated,\n> > > > > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result.\n> > > > >\n> > > > > Signed-off-by: Han-Lin Chen \n> > > > > Co-developed-by: Harvey Yang \n> > > > > Signed-off-by: Harvey Yang \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 17 ++++++++++++-----\n> > > > > 1 file changed, 12 insertions(+), 5 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index 497d363d6..dd2c603e0 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > >\n> > > > You should update the comment before the if()\n> > >\n> > > Updated. Please take another look.\n> > >\n> > > >\n> > > > > << \" not successfully completed: \"\n> > > > > << request->status();\n> > > > >\n> > > > > - abortRequest(descriptor);\n> > > > > - completeDescriptor(descriptor);\n> > > > > -\n> > > > > - return;\n> > > >\n> > > > Is it ok to continue and try to fetch metadata from a Request in error\n> > > > status ?\n> > >\n> > > I think so, as libcamera core library sets a Request in error status\n> > > if and only if there's a buffer in error status. There might be some\n> > > valid metadata in the Request.\n> > >\n>\n> I guess the major takeaway here is that the libcamera error\n> notification is really not sophisticated enough.\n\nExactly :)\n\n>\n> > > >\n> > > > Also, the\n> > > >\n> > > > if (request->status() != Request::RequestComplete) {\n> > > >\n> > > > case, doesn't fall in\n> > > >\n> > > > /**\n> > > > * S6. Error management:\n> > > > *\n> > > >\n> > > > ...\n> > > >\n> > > > * - The failure of an entire capture to occur must be reported by the HAL by\n> > > > * calling notify() with ERROR_REQUEST. Individual errors for the result\n> > > > * metadata or the output buffers must not be reported in this case.\n> > > >\n> > > > I think the\n> > > >\n> > > > abortRequest(descriptor);\n> > > > completeDescriptor(descriptor);\n> > > >\n> > > > sequence should be kept here ?\n> > >\n> > > Not really. Perhaps we should update `camera3.h`, while this is what I\n> > > wanted to discuss with you in the previous version of this patch:\n> > >\n> > > CAMERA3_MSG_ERROR_REQUEST is mostly used for requests that haven't\n> > > done any processing.\n> > > CAMERA3_MSG_ERROR_RESULT is used for requests that have missing metadata.\n> > >\n> > > A request in error status doesn't fall into either of the cases.\n> > >\n>\n> Right\n>\n> > > If you can take another look at my last comments in `[PATCH v2 6/9]\n> > > android: Cleanup CAMERA3_MSG_ERROR_REQUEST`, we can align and discuss\n> > > how to proceed here. Thanks :)\n> > >\n> > > Ref: https://source.android.com/reference/hal/structcamera3__device__ops\n> > >\n>\n> Yes, sorry I missed that and jumped on this version.\n>\n> I see your point: The RequestCancelled state doesn't tell us nothing\n> about metadata, but only that some buffer has failed.\n>\n> However, this means some processing has been done, and you're right\n> this should be transmitted to Android with an ERROR_RESULT code\n>\n> > > >\n> > > > Specifically, if you don't call completeDescriptor() the descriptor\n> > > > will remain in pending state and stall the queue of complete\n> > > > descriptors\n> > >\n> > > As we don't return in this if clause, either there is at least one\n> > > buffer to be post-processed, or completeDescriptor() will be called on\n> > > the descriptor. Let me know if I misunderstand anything.\n>\n> This is the part that scary me a bit.\n>\n> When we try to post-process we don't check for the source buffer\n> validity, am I wrong ? I would be fine with your approach, but if we\n> continue processing the Request even if it's in Cancelled state, then\n> any access to the buffers should be checked and handled correctly ?\n\nAh, thanks for the catch!\n\nI think we need to set the buffers that depend on a cancelled source\nbuffer to be StreamBuffer::Status::Error as well.\n\n>\n> > >\n> > > >\n> > > >\n> > > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > > > }\n> > > > >\n> > > > > /*\n> > > > > @@ -1239,7 +1236,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > > > */\n> > > > > descriptor->resultMetadata_ = getResultMetadata(*descriptor);\n> > > > > if (!descriptor->resultMetadata_) {\n> > > > > - notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > >\n> > > > To be honest I don't see what we gain by delaying the notification\n> > >\n> > > We can also remove the logic entirely: getResultMetadata() only fails\n> > > to return a valid CameraMetadata with memory issues I think, not due\n> > > to some corrupted Request::metadata(). It will be removed in the end\n>\n> Well, failure to reserve space is a metadata failure, isn't it ?\n\nWell... fair... Delaying the notification is just to simplify the\nlogic, as it might happen along with a cancelled request as well, so\nthat we don't send the signal twice.\n\n>\n> > > anyways.\n> > >\n> > > WDYT?\n> > >\n>\n> Thing is, to post-process you need metadata, don't you ? Is it worth\n> trying to post-process without metadata ?\n\nI think it's a different thing: Failing to allocate memory for\nCameraMetadata doesn't mean that there are some metadata missing\nduring the processing of the pipeline handler. True that currently\nJPEG post processor reads from `resultMetadata` to fill in jpeg\nmetadata, while that might not always be the case. In the following\npatches, we'll provide some metadata with the new signal, right?\n\nAs long as the current implementation of post processors don't fall\ninto fatal errors, I think we can keep it this way. WDYT?\n(The original logic also allows post-processing when\n`getResultMetadata()` fails, doesn't it?\n\n>\n> > > >\n> > > >\n> > > > >\n> > > > > /*\n> > > > > * The camera framework expects an empty metadata pack on error.\n> > > > > @@ -1325,6 +1322,16 @@ void CameraDevice::sendCaptureResults()\n> > > > > descriptors_.pop();\n> > > > >\n> > > > > sendCaptureResult(descriptor.get());\n> > > > > +\n> > > > > + /*\n> > > > > + * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some\n> > > > > + * of the expected result metadata might not be available\n> > > > > + * because the capture is cancelled by the camera. Only notify\n> > > > > + * it when the final result is sent, since Android will ignore\n> > > > > + * the following metadata.\n> > > > > + */\n> > > > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)\n> > > > > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);\n> > > >\n> > > > If my understanding of the specs if correct, I think in case of\n> > > >\n> > > > if (request->status() != Request::RequestComplete) {\n> > > >\n> > > > the right error code to notify is ERROR_REQUEST, so if my\n> > > > understanding is correct, I would rather drop this whole change.\n> > > >\n> > > > Or am I mistaken maybe ? Have you got any failure in CTS or other\n> > > > tests because of this ?\n> > >\n> > > Yeah that's what I wanted to fix with this patch:\n> > > ```\n> > > 3.4 For captures that will produce some results, the HAL must not call\n> > > CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure.\n> > > ```\n> > >\n> > > Previously, we just ignored any potentially available metadata and\n> > > buffers and notified CAMERA3_MSG_ERROR_REQUEST on a request in error\n> > > status. This doesn't seem correct to me, while I believe it still fits\n> > > in the Android API.\n>\n> Ok, I'll take this as we don't have expressive enough failures in\n> libcamera. Error handling paths are particularly hard to test, that's\n> why I'm so cautious here, before this patch we considered\n> RequestCancelled == Complete Request Failure and we didn't deliver\n> anything to the framework for this request.\n>\n> Moving forward some metadata might be notified to the framework before\n> requestComplete() so ERROR_REQUEST is not the right code anymore.\n>\n> I'm fine with this (now that I understand it) but please let me know\n> what do you think about checking the source buffer status when doing\n> post-processing.\n>\n> > >\n> > > Regarding failure in CTS or other tests, I just tried on mtkisp7.\n> > > Without this patch, CTS still passes. I think it should be fine\n> > > without this patch, if it's controversial.\n> >\n> > Sorry, I take it back. I believe mtkisp7 passed CTS because we didn't\n> > produce any failure.\n>\n> Yes, failure path are particularly hard to test, I know...\n>\n>\n> >\n> > In Android:\n> > ```\n> > For captures that will produce some results, the HAL must not call\n> > CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure.\n> > ```\n> >\n> > With partial results, it cannot be guaranteed that there was no buffer\n> > / metadata completed and sent to the application before we receive the\n> > requestCompleted signal. We have to deal with this issue before the\n> > partial result patches land.\n>\n> I agree. Thanks for explaining, let me know what you think about the\n> last missing item (checking src buffer state when post-processing)\n\nYes, I'll upload a fix for that in the next version.\n\nBR,\nHarvey\n\n>\n> Thanks\n> j\n>\n> >\n> > BR,\n> > Harvey\n> >\n> > >\n> > > BR,\n> > > Harvey\n> > >\n> > > >\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > --\n> > > > > 2.47.0.338.g60cca15819-goog\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E9A4BBD80A\n\tfor ;\n\tTue, 10 Dec 2024 09:19:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D42C67E80;\n\tTue, 10 Dec 2024 10:19:05 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 52543608B6\n\tfor ;\n\tTue, 10 Dec 2024 10:19:03 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id\n\t38308e7fff4ca-3003e203acaso26161371fa.1\n\tfor ;\n\tTue, 10 Dec 2024 01:19:03 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"GZdrZupm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733822342; x=1734427142;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=YGHycgNRWqgpQIG5BQCTAxdus3V4jRpVspLPHbbW+Z4=;\n\tb=GZdrZupmmS6CCdoi60aACMzRfDtP8t2CoM5XFSSe6x6NQxpVZZ11nmc43tLA/d8Lkl\n\tzAFh+i4X4bhSA299girMIs8rS8lN7sfk/hBeh5u/cDrNA6usp4MOrBt3cBQpOHjGk7qz\n\tDQfq+VgPEvCVn5zSdYToNEKKO2kie8dYWfPZ8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733822342; x=1734427142;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=YGHycgNRWqgpQIG5BQCTAxdus3V4jRpVspLPHbbW+Z4=;\n\tb=rH1tWHC9lYHB/RuUkq8of5zC8Llza1st3yNQQHCGoyxKy7yx+32vV5DQoE8yYhfb9L\n\t4xxTUzk91d9usQIsVOFDF7/hW/GRTVIJFaoSixBpCpv7BRJFiXipE4lAwg6neQR1r/UN\n\tawgHh2t38aaytyqHNdZHY7LNL7D7FiQlUoQR+gRmbwHvyxsaPHLEF8t8cmOvYs6wU/u9\n\twcvgVSYKAL8bduOC4ma+i4C9eJnBlklUBCxu0qEY7nM1GJJe6/Z97QD8ECqBEzCxNNIZ\n\tlSX0FwxM+JPaPN8Xx1Gv5WA3m4ADnFhOENJymOq5ydNpdPteNdGM8k/LsLfkLcQPvpDq\n\tOIAA==","X-Gm-Message-State":"AOJu0YxQEBsHsQMS/FGo3p8QW5GNnRIsvWzxps5w2+5t5gehZ1gB/vCm\n\tFcbEjozJuHBGm5hhYJtvytJ3xC2hDo2mVYcIjxGbhlhmzqO2Oww3X1dMdzTPr6L3otulhnXW4tp\n\tUpuMXQUg2KB/YIUMo+j/dbY5cjTle7apgVS8O","X-Gm-Gg":"ASbGnct4W9HvMuwscQxIPO9ebHxfmROPxYUrezD808Odxvux2RRkOT0tTrlBO+ZaN5I\n\t51OLShRCLqarlUR7PfbmvbNSTqcLB7rZXyk9Rm5RICxF9TjLCzeC09sfE68IA","X-Google-Smtp-Source":"AGHT+IEJc79Roa8mp2v1bz0Ck9q0NNpjVjsfJVT2uztFUaL0ZICwXalj9OHgUS9A/0Q0nA+88WZ1YX8Z01FfazZRuQ8=","X-Received":"by 2002:a05:651c:1587:b0:2ff:d83d:9155 with SMTP id\n\t38308e7fff4ca-3022fd87c3fmr14567271fa.27.1733822342416;\n\tTue, 10 Dec 2024 01:19:02 -0800 (PST)","MIME-Version":"1.0","References":"<20241204164137.3938891-1-chenghaoyang@chromium.org>\n\t<20241204164137.3938891-6-chenghaoyang@chromium.org>\n\t<6i6a56jpiju7dawyypd4az64xryck73b6pu5kjck3an7zcsygy@kekkjw2yb53x>\n\t\n\t\n\t<2wckutph6ccrlqr2czgwq5eelxnmyte7bo3ol2y7ocm6opxqjk@xhcubkyqtpoo>","In-Reply-To":"<2wckutph6ccrlqr2czgwq5eelxnmyte7bo3ol2y7ocm6opxqjk@xhcubkyqtpoo>","From":"Cheng-Hao Yang ","Date":"Tue, 10 Dec 2024 17:18:51 +0800","Message-ID":"","Subject":"Re: [PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]