[{"id":32437,"web_url":"https://patchwork.libcamera.org/comment/32437/","msgid":"","date":"2024-11-28T15:02:33","subject":"Re: [PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST","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, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote:\n> When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT\n> should be used instead.\n>\n> This patch also cleans up aborting when flushing with\n> sendCaptureResult().\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 | 28 +++++++++++++++-------------\n> 1 file changed, 15 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 0377cf215..3fb92268e 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> \t\tbuffer.status = StreamBuffer::Status::Error;\n>\n> \tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> +\n> +\tsendCaptureResult(descriptor);\n> }\n>\n> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tMutexLocker stateLock(stateMutex_);\n>\n> \tif (state_ == State::Flushing) {\n> -\t\tCamera3RequestDescriptor *rawDescriptor = descriptor.get();\n> -\t\t{\n> -\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\t\t\tdescriptors_.push(std::move(descriptor));\n> -\t\t}\n> -\t\tabortRequest(rawDescriptor);\n> -\t\tcompleteDescriptor(rawDescriptor);\n> -\n> +\t\tabortRequest(descriptor.get());\n\nI'm not sure I get this change ?\n\nSpecifically why we don't complete the descriptor anymore..\n\n\n> \t\treturn 0;\n> \t}\n>\n> @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request)\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> +\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> \t}\n>\n> \t/*\n> @@ -1239,7 +1231,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>\n> \t\t/*\n> \t\t * The camera framework expects an empty metadata pack on error.\n> @@ -1325,6 +1317,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> \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 2BC4FBDE6B\n\tfor ;\n\tThu, 28 Nov 2024 15:02:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08A5865FB2;\n\tThu, 28 Nov 2024 16:02:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8FD6065898\n\tfor ;\n\tThu, 28 Nov 2024 16:02:37 +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 96FD8526;\n\tThu, 28 Nov 2024 16:02:13 +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=\"HqlR0QOi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732806133;\n\tbh=XDjw3+kLQY3NeJX0QFP52O4BH+w6Fs4AxF6YP5cvzq0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HqlR0QOigpfZUnqt3gwVNQiEweGZteF/6QVVeEJpyJ3aZv3B9lm7BJH+nfxOFtAfY\n\tBJy345Vf61j+AOPZE4JNKhytruN0ljjGCx/ptC1yaMXFlpAI+U72AOZJaTf2fN/MSm\n\t+tkpDkNmPd/USGM/Qw8pubZF5sKyfRU9fXFc4PlQ=","Date":"Thu, 28 Nov 2024 16:02:33 +0100","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST","Message-ID":"","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-7-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241127092632.3145984-7-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":32438,"web_url":"https://patchwork.libcamera.org/comment/32438/","msgid":"","date":"2024-11-28T15:30:15","subject":"Re: [PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Nov 28, 2024 at 11:02 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote:\n> > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT\n> > should be used instead.\n> >\n> > This patch also cleans up aborting when flushing with\n> > sendCaptureResult().\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 | 28 +++++++++++++++-------------\n> > 1 file changed, 15 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 0377cf215..3fb92268e 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> > buffer.status = StreamBuffer::Status::Error;\n> >\n> > descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > +\n> > + sendCaptureResult(descriptor);\n> > }\n> >\n> > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > MutexLocker stateLock(stateMutex_);\n> >\n> > if (state_ == State::Flushing) {\n> > - Camera3RequestDescriptor *rawDescriptor = descriptor.get();\n> > - {\n> > - MutexLocker descriptorsLock(descriptorsMutex_);\n> > - descriptors_.push(std::move(descriptor));\n> > - }\n> > - abortRequest(rawDescriptor);\n> > - completeDescriptor(rawDescriptor);\n> > -\n> > + abortRequest(descriptor.get());\n>\n> I'm not sure I get this change ?\n>\n> Specifically why we don't complete the descriptor anymore..\n\nOkay, apart from the fix of CAMERA3_MSG_ERROR_RESULT\ninstead of CAMERA3_MSG_ERROR_REQUEST, this patch\ncontains one more thing that is only briefly mentioned in the\ncommit message:\n\nWhen flushing and aborting new requests, we should notify\nCAMERA3_MSG_ERROR_REQUEST, and call\n`process_capture_result` earlier, instead of waiting for the\nprevious requests being completed first.\n\n`completeDescriptor` would trigger the in-order completion\nof requests only, while in this patch, we call\n`sendCaptureResult` directly, without the necessity to\nset `complete_` anymore.\n\nHope it helps explain.\n\nBR,\nHarvey\n\n>\n>\n> > return 0;\n> > }\n> >\n> > @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request)\n> > << \" not successfully completed: \"\n> > << request->status();\n> >\n> > - abortRequest(descriptor);\n> > - completeDescriptor(descriptor);\n> > -\n> > - return;\n> > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > }\n> >\n> > /*\n> > @@ -1239,7 +1231,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> > /*\n> > * The camera framework expects an empty metadata pack on error.\n> > @@ -1325,6 +1317,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> > }\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 34AA0BD78E\n\tfor ;\n\tThu, 28 Nov 2024 15:30:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36B4F65FBD;\n\tThu, 28 Nov 2024 16:30:30 +0100 (CET)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D81065898\n\tfor ;\n\tThu, 28 Nov 2024 16:30:27 +0100 (CET)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2ffdd9fc913so10171791fa.3\n\tfor ;\n\tThu, 28 Nov 2024 07:30:27 -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=\"SEjHQ1sX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1732807826; x=1733412626;\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=uNzENVITckmgLkF4kCiKR8teEAFbVc5jdBSCZ/odvyQ=;\n\tb=SEjHQ1sX+dvrSpZCGMLc7dTiL6/t6D8ZM1splaB6cTvmTI2LQOKUlgOtaJVfnmAI7v\n\tforBYmNycLKScV51LJU/sl/aDsPCZKxCN9WI1Jr5KaHFaJmYyP5kazewUeAhLuREHpf9\n\tc+zNGY89xyKq9VWgimv5BeX2bMLkN1wprTLvQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732807826; x=1733412626;\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=uNzENVITckmgLkF4kCiKR8teEAFbVc5jdBSCZ/odvyQ=;\n\tb=bUFy/DrHEDBdbvBSqTQxUbmBgK+BkcRZRRdYa6GwcSIw5CvlzGYFGI6aG4FmiQ/MaM\n\tib+IcWNzWDFVN1e3MTcLOumBzgVsTcNKsY/P5gWcWnzjpofiFQtbX3Jo6JnrHb4AA1Kd\n\taEwg3ZNzFL2HTduyum1s+Kug9KI/KuwWlrfz+june/Aqt2wHgVAA4Onp8h9NA5hBOu+P\n\tvK6nLOj/PMJ/vBpzP0zTGJgW21BxGbi8WlnMxeMXzztzhis7L8Q77XNmV66H+SE/zhnW\n\twxBRaDmML3RAQhoEUiDTxUEylBG8nSoMuNXFQrLStRVJ+vkrTU/6WHeulW/thpAixZyr\n\tUhzQ==","X-Gm-Message-State":"AOJu0YwvjwY85Wi5csHwxhYCGd17U9FNi2UA9WdGnt/QmG3oMDpQdiPC\n\ttuxM9Ft5mEliI7muNKhsuJSw6Vc9btyixJnGO8z/QkodXBYwtNAr90TjFREk9PPEx2iDA52wx8T\n\tkP07cNOto1w5htNrsr9yhBTQdEI4SpugqxPLg","X-Gm-Gg":"ASbGnctIzZAMYZxT2m4fc15pYs9D+qsYSBkZtrzJe8ExWqhvanYDOP1MKQCPpkiyoDL\n\tRe/AL6OP3KhO0pa62EigsAADzHD7SeZbeuHuXAMS5CYCa6tB3QINdVilFZmdcvx0Yxg==","X-Google-Smtp-Source":"AGHT+IEDo9pqXl6jv7hmiY7zCeyn1xUJJfeIkvyXtJAl4bFPvYS3LcqpqvzU5Qkju6BBXK+F4aFyaRSVS7rhdbhBMHE=","X-Received":"by 2002:a05:651c:198d:b0:2ff:d044:61fa with SMTP id\n\t38308e7fff4ca-2ffd60e2054mr48724611fa.36.1732807826249;\n\tThu, 28 Nov 2024 07:30:26 -0800 (PST)","MIME-Version":"1.0","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-7-chenghaoyang@chromium.org>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 28 Nov 2024 23:30:15 +0800","Message-ID":"","Subject":"Re: [PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST","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":32486,"web_url":"https://patchwork.libcamera.org/comment/32486/","msgid":"","date":"2024-12-02T16:17:52","subject":"Re: [PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Thu, Nov 28, 2024 at 11:30:15PM +0800, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> On Thu, Nov 28, 2024 at 11:02 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Harvey\n> >\n> > On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote:\n> > > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT\n> > > should be used instead.\n> > >\n> > > This patch also cleans up aborting when flushing with\n> > > sendCaptureResult().\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 | 28 +++++++++++++++-------------\n> > > 1 file changed, 15 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 0377cf215..3fb92268e 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> > > buffer.status = StreamBuffer::Status::Error;\n> > >\n> > > descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > +\n> > > + sendCaptureResult(descriptor);\n> > > }\n> > >\n> > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> > > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > MutexLocker stateLock(stateMutex_);\n> > >\n> > > if (state_ == State::Flushing) {\n> > > - Camera3RequestDescriptor *rawDescriptor = descriptor.get();\n> > > - {\n> > > - MutexLocker descriptorsLock(descriptorsMutex_);\n> > > - descriptors_.push(std::move(descriptor));\n> > > - }\n> > > - abortRequest(rawDescriptor);\n> > > - completeDescriptor(rawDescriptor);\n> > > -\n> > > + abortRequest(descriptor.get());\n> >\n> > I'm not sure I get this change ?\n> >\n> > Specifically why we don't complete the descriptor anymore..\n>\n> Okay, apart from the fix of CAMERA3_MSG_ERROR_RESULT\n> instead of CAMERA3_MSG_ERROR_REQUEST, this patch\n\nWhere did this happen ? In flush ? I read it used to call\nabortRequest() that\n\n\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n\nand used to call\n\nvoid CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n{\n\tMutexLocker lock(descriptorsMutex_);\n\tdescriptor->complete_ = true;\n\n\tsendCaptureResults();\n}\n\nSo I might have missed what do we gain with this change (as\nabortRequest() has a single call place if I'm not mistaken.\nAlso, this new version won't set\n\n\tdescriptor->complete_ = true;\n\nas the previous call to completeDescriptor() did.\n\n> contains one more thing that is only briefly mentioned in the\n> commit message:\n>\n> When flushing and aborting new requests, we should notify\n> CAMERA3_MSG_ERROR_REQUEST, and call\n> `process_capture_result` earlier, instead of waiting for the\n> previous requests being completed first.\n>\n> `completeDescriptor` would trigger the in-order completion\n> of requests only, while in this patch, we call\n> `sendCaptureResult` directly, without the necessity to\n> set `complete_` anymore.\n>\n> Hope it helps explain.\n\nAh yes, thanks, so we can't go through completeDescriptor() because it\ncompletes in-order but you need to to go through a direct call to\nsendCaptureResult().\n\nThanks, it helps. I would have helped even more if you have added it\nto the commit message, because this\n\n When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT\n should be used instead.\n\n This patch also cleans up aborting when flushing with\n sendCaptureResult().\n\ndoesn't explain me anything and we had to go through three emails and\nme trying to understand what you have done. Spending 5 more minutes to\nwrite a proper commit message would have avoided that.\n\n>\n> BR,\n> Harvey\n>\n> >\n> >\n> > > return 0;\n> > > }\n> > >\n> > > @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > << \" not successfully completed: \"\n> > > << request->status();\n> > >\n> > > - abortRequest(descriptor);\n> > > - completeDescriptor(descriptor);\n> > > -\n> > > - return;\n> > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > }\n> > >\n> > > /*\n> > > @@ -1239,7 +1231,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> > > /*\n> > > * The camera framework expects an empty metadata pack on error.\n> > > @@ -1325,6 +1317,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\nIs this part of the patch related to the above ? What happens here ?\nWhy do you delay the call to notifyError() ?\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 82BE1BDC71\n\tfor ;\n\tMon, 2 Dec 2024 16:17:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C783766070;\n\tMon, 2 Dec 2024 17:17: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 861D56605F\n\tfor ;\n\tMon, 2 Dec 2024 17:17:56 +0100 (CET)","from ideasonboard.com (mob-5-90-236-68.net.vodafone.it\n\t[5.90.236.68])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 322D76DE;\n\tMon, 2 Dec 2024 17:17:29 +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=\"sUdUxanH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733156249;\n\tbh=dy1eJKTIYuEEdB7jZIOYZsl4QFCpD/mSjp4c6ypnZY0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sUdUxanHHvNxXkb9t5nKU8X5SsEYZeIf9tb7Tne8tkr2YdyfyjXCZD1R2Yljwnh1F\n\tjgZ6ipy1Rc/W3Z/nASmwrXbOQfCzeJ/w3ZII8idE+K45GH/b4Xi3D92rNC8iaw1VKk\n\tlDcdt6z1VI7l+dI7mEAdzBop8c7r6aesyrgT4jrU=","Date":"Mon, 2 Dec 2024 17:17:52 +0100","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST","Message-ID":"","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-7-chenghaoyang@chromium.org>\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":32492,"web_url":"https://patchwork.libcamera.org/comment/32492/","msgid":"","date":"2024-12-03T10:21:34","subject":"Re: [PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST","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 3, 2024 at 12:17 AM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Thu, Nov 28, 2024 at 11:30:15PM +0800, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > On Thu, Nov 28, 2024 at 11:02 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Harvey\n> > >\n> > > On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote:\n> > > > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT\n> > > > should be used instead.\n> > > >\n> > > > This patch also cleans up aborting when flushing with\n> > > > sendCaptureResult().\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 | 28 +++++++++++++++-------------\n> > > > 1 file changed, 15 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 0377cf215..3fb92268e 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const\n> > > > buffer.status = StreamBuffer::Status::Error;\n> > > >\n> > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > > +\n> > > > + sendCaptureResult(descriptor);\n> > > > }\n> > > >\n> > > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> > > > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > MutexLocker stateLock(stateMutex_);\n> > > >\n> > > > if (state_ == State::Flushing) {\n> > > > - Camera3RequestDescriptor *rawDescriptor = descriptor.get();\n> > > > - {\n> > > > - MutexLocker descriptorsLock(descriptorsMutex_);\n> > > > - descriptors_.push(std::move(descriptor));\n> > > > - }\n> > > > - abortRequest(rawDescriptor);\n> > > > - completeDescriptor(rawDescriptor);\n> > > > -\n> > > > + abortRequest(descriptor.get());\n> > >\n> > > I'm not sure I get this change ?\n> > >\n> > > Specifically why we don't complete the descriptor anymore..\n> >\n> > Okay, apart from the fix of CAMERA3_MSG_ERROR_RESULT\n> > instead of CAMERA3_MSG_ERROR_REQUEST, this patch\n>\n> Where did this happen ? In flush ? I read it used to call\n> abortRequest() that\n>\n> notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>\n> and used to call\n>\n> void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)\n> {\n> MutexLocker lock(descriptorsMutex_);\n> descriptor->complete_ = true;\n>\n> sendCaptureResults();\n> }\n>\n> So I might have missed what do we gain with this change (as\n> abortRequest() has a single call place if I'm not mistaken.\n> Also, this new version won't set\n>\n> descriptor->complete_ = true;\n>\n> as the previous call to completeDescriptor() did.\n\nSeems that you got most of the ideas, regarding your following\ncomments? Let me know if you still have questions :)\n\n>\n> > contains one more thing that is only briefly mentioned in the\n> > commit message:\n> >\n> > When flushing and aborting new requests, we should notify\n> > CAMERA3_MSG_ERROR_REQUEST, and call\n> > `process_capture_result` earlier, instead of waiting for the\n> > previous requests being completed first.\n> >\n> > `completeDescriptor` would trigger the in-order completion\n> > of requests only, while in this patch, we call\n> > `sendCaptureResult` directly, without the necessity to\n> > set `complete_` anymore.\n> >\n> > Hope it helps explain.\n>\n> Ah yes, thanks, so we can't go through completeDescriptor() because it\n> completes in-order but you need to to go through a direct call to\n> sendCaptureResult().\n>\n> Thanks, it helps. I would have helped even more if you have added it\n> to the commit message, because this\n>\n> When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT\n> should be used instead.\n>\n> This patch also cleans up aborting when flushing with\n> sendCaptureResult().\n>\n> doesn't explain me anything and we had to go through three emails and\n> me trying to understand what you have done. Spending 5 more minutes to\n> write a proper commit message would have avoided that.\n\nYeah sorry, I'll do better next time...\n\n>\n> >\n> > BR,\n> > Harvey\n> >\n> > >\n> > >\n> > > > return 0;\n> > > > }\n> > > >\n> > > > @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > > << \" not successfully completed: \"\n> > > > << request->status();\n> > > >\n> > > > - abortRequest(descriptor);\n> > > > - completeDescriptor(descriptor);\n> > > > -\n> > > > - return;\n> > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > > }\n> > > >\n> > > > /*\n> > > > @@ -1239,7 +1231,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> > > > /*\n> > > > * The camera framework expects an empty metadata pack on error.\n> > > > @@ -1325,6 +1317,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> Is this part of the patch related to the above ? What happens here ?\n> Why do you delay the call to notifyError() ?\n\nActually there are quite some things to discuss here: We don't think\nthe way tot is implemented that notifying CAMERA3_MSG_ERROR_RESULT\nwhen `getResultMetadata()` fails make sense, as `getResultMetadata()`\nonly fails when we fail to allocate memory for CameraMetadata IIUC. In\nthe last patch, you can find that this logic is removed. We may also\nconsider removing this logic with an extra patch.\n\nThis patch calls CAMERA3_MSG_ERROR_RESULT when the libcamera::Request\nwas cancelled, which will happen when any buffer was cancelled [1].\nIt's quite different from CAMERA3_MSG_ERROR_RESULT's description [2]\nthough, which indicates that there is some/all metadata missing.\nCurrently we don't check the absence of metadata and report an error,\nyet.\nMy question is: Do you think that simplifying and taking\nRequestCancelled as CAMERA3_MSG_ERROR_RESULT makes sense?\n\nWe can also not keep it as CAMERA3_MSG_ERROR_REQUEST, because we're\ntrying to support partial results, and CAMERA3_MSG_ERROR_REQUEST is\ncurrently triggered in signal requestCompleted, and it's for \"pending\nrequests that have not done any processing\". Having completed partial\nresults and CAMERA3_MSG_ERROR_REQUEST doesn't make much sense to me\n(although according to the description, it's not restricted as well?\nI'm not sure...)\n\nWDYT?\n\nBR,\nHarvey\n\n[1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/request.cpp#n108\n[2]: https://source.android.com/reference/hal/structcamera3__device__ops\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 916BBBDCBF\n\tfor ;\n\tTue, 3 Dec 2024 10:21:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9443766081;\n\tTue, 3 Dec 2024 11:21:47 +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 3B2CE66072\n\tfor ;\n\tTue, 3 Dec 2024 11:21:46 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id\n\t38308e7fff4ca-2ff976ab0edso53805581fa.1\n\tfor ;\n\tTue, 03 Dec 2024 02:21:46 -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=\"JDTUVkkk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733221305; x=1733826105;\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=o2tL4Kdf27ir5b1zYgc+KlsF/6DqX3zm4BmrGcXrNYE=;\n\tb=JDTUVkkk4zsSjoTxlVl2dBUgUdVHNHg0RjtBChZin1jS0yzcOZAPg8s7qkM/AwOkVV\n\tL1ynEiznGJckzq+epi3edyiW/8dDr3yP1sOK6uSBrQqzpVq5s9sr87I2KLGBdkx46acR\n\t8vMCDMS2SmseKraqciza2vr2Lu57JsPMXhRJU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733221305; x=1733826105;\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=o2tL4Kdf27ir5b1zYgc+KlsF/6DqX3zm4BmrGcXrNYE=;\n\tb=PPm9ArBiADFojA4NYiugIraKjMB/EwTnbkZ6GvhnAwy+YxKg7liUk4nGN1eOulLFB+\n\tekxDuPRY5BRT9a2jN5FSolJAamTYVP+W3i9YDZwzlOqLshG2U9JoUwWkIVvDNoSUncLN\n\tCmyRZzRzNiWp/dfpsSioyOi4BHyumXyw7wXeVa5ce8nRFt+xcjQOOokfAYMES8noUqBN\n\tB2cZOa6qHE2bhf8g1aWGQPEDi0S/gMpI31seCvu1OxYuyruKV7+zfCKClo+NVf0ppvTr\n\tVkfNP+03Db0M3l7Q/Y7WkCBmeRhI7uX2uOnjCFwtOHl6WpD9WyHkb3nefXVJXMTo2W96\n\tGVmA==","X-Gm-Message-State":"AOJu0YyZgegdBCXkDYL1Vow0mzuWbBp19WhiWKod/vWUK0EgMaG+WytI\n\t8tBQv5zKjt0pBv8rJlIKoGJMZKYXFiUq9i8TiNvtOsflWgLSLdGKDXiURhyG5Rjuh/kL8xpBQKn\n\tZubytV/hge4NoPH4lmH4TCPgTkKcIR1GN5KVr","X-Gm-Gg":"ASbGncsRGNGM2ldKrazuCpBzfqCaRLrRmXpUSD6B6ObBf6CxGa0xImxbScqF1XkHwWq\n\tDXeqTUCN8d4w0xPu+JMZP4C0rcHlErl+G7p6g/GdJ7DiLT5o3arCAhXtHvbw=","X-Google-Smtp-Source":"AGHT+IF86IZy3tnb5lbOGHwi7dtyseZum0UH55GxohuiXCehBlchkV0qy2KVgs7bkAD1mJGtqdZmuF+5BGM5DBLP8Es=","X-Received":"by 2002:a05:651c:b14:b0:2ff:dd4d:df7b with SMTP id\n\t38308e7fff4ca-30009c9239emr11743571fa.27.1733221305199;\n\tTue, 03 Dec 2024 02:21:45 -0800 (PST)","MIME-Version":"1.0","References":"<20241127092632.3145984-1-chenghaoyang@chromium.org>\n\t<20241127092632.3145984-7-chenghaoyang@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Tue, 3 Dec 2024 18:21:34 +0800","Message-ID":"","Subject":"Re: [PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST","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\" "}}]