[{"id":32673,"web_url":"https://patchwork.libcamera.org/comment/32673/","msgid":"","date":"2024-12-11T11:06:21","subject":"Re: [PATCH v4 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 Tue, Dec 10, 2024 at 02:23:58PM +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 | 40 ++++++++++++++++++++++++++++-------\n> 1 file changed, 32 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f3f570544..3b10f207e 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1182,6 +1182,18 @@ void CameraDevice::requestComplete(Request *request)\n> \t * post-processing/compression fails.\n> \t */\n> \tfor (auto &buffer : descriptor->buffers_) {\n> +\t\tfor (auto &[_, frameBuffer] : request->buffers()) {\n\nI've been thinking about ways to avoid the double lookup here, we\ndon't have that many requests or descriptor, but avoiding doing it for every\nrequest might be desirable.\n\nI've been trying to do so by setting a pointer to a StreamBuffer as\nlibcamera::FrameBuffer::cookie and retrieve it here. However, as\nnow multiple Android streams can map to the same FrameBuffer there's\nno 1-to-1 association anymore between a StreamBuffer and a\nFrameBuffer...\n\nToo bad, I would have liked to avoid this\n\n> +\t\t\tif (buffer.srcBuffer != frameBuffer &&\n> +\t\t\t buffer.frameBuffer.get() != frameBuffer)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tStreamBuffer::Status status = StreamBuffer::Status::Success;\n> +\t\t\tif (frameBuffer->metadata().status != FrameMetadata::FrameSuccess) {\n> +\t\t\t\tstatus = StreamBuffer::Status::Error;\n> +\t\t\t}\n\nNo {} for single line statements\n\n> +\t\t\tsetBufferStatus(buffer, status);\n> +\t\t}\n> +\n> \t\tCameraStream *stream = buffer.stream;\n\nCan you move this after the comment block if you have to resend ?\n\n>\n> \t\t/*\n> @@ -1198,22 +1210,18 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tif (fence)\n> \t\t\t\tbuffer.fence = fence->release();\n> \t\t}\n> -\t\tbuffer.status = StreamBuffer::Status::Success;\n> \t}\n>\n> \t/*\n> -\t * If the Request has failed, abort the request by notifying the error\n> -\t * and complete the request with all buffers in error state.\n> +\t * If the Request has failed, complete the request with all buffers in\n> +\t * error state and notify an error result.\n\nWith this patch buffers are completed indivdually in error state just\nbelow. I would move the below if() before inspecting the single\nbuffers or even drop it completely and just rely on the above loop, after\nall if one buffer has failed the whole request has failed and you now\nset its state to Camera3RequestDescriptor::Status::Error in the above\ncall to setBufferStatus()\n\n> \t */\n> \tif (request->status() != Request::RequestComplete) {\n> \t\tLOG(HAL, Error) << \"Request \" << request->cookie()\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> @@ -1238,7 +1246,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> @@ -1271,7 +1279,13 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n>\n> +\t\tif (buffer->status == StreamBuffer::Status::Error) {\n> +\t\t\titer = descriptor->pendingStreamsToProcess_.erase(iter);\n> +\t\t\tcontinue;\n> +\t\t}\n\nnit: could you move this before the previous if() ? There's no point\nin searching the FrameBuffer if we know the stream has failed ?\n\n> +\n> \t\t++iter;\n> +\n> \t\tint ret = stream->process(buffer);\n> \t\tif (ret) {\n> \t\t\tsetBufferStatus(*buffer, StreamBuffer::Status::Error);\n> @@ -1324,6 +1338,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\nmetdata and buffers ?\n\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\nI would drop \"since ...\"\n\nAll minors, next one should be good to go\n\nThanks\n j\n\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 1FDB8C32DD\n\tfor ;\n\tWed, 11 Dec 2024 11:06:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C21367EA9;\n\tWed, 11 Dec 2024 12:06:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDF17618AE\n\tfor ;\n\tWed, 11 Dec 2024 12:06:24 +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 CC036352;\n\tWed, 11 Dec 2024 12:05:51 +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=\"gGyJ9sF8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733915151;\n\tbh=FJBQ4lhCLTuBRMUczIr0e0f063b7gNSN9f79v8u93PA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gGyJ9sF80DdweOmydggwArTeK55Dr3Xd6tQprcORMIKRqVYjRLMYz9BN/fQgh6FLM\n\t78etMMoxeWfIfxDyKcFq8O+G3eoqktf4lZYaXVwMJW8QAb/b+TmvF+DyRctAlQCDvr\n\tq0LFFLnkieKz3KiQF8VYC8SR+DOAUoCv3F+pWYZA=","Date":"Wed, 11 Dec 2024 12:06:21 +0100","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Subject":"Re: [PATCH v4 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST\n\twhen a request fails","Message-ID":"","References":"<20241210142557.2886315-1-chenghaoyang@chromium.org>\n\t<20241210142557.2886315-6-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241210142557.2886315-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":32687,"web_url":"https://patchwork.libcamera.org/comment/32687/","msgid":"","date":"2024-12-12T09:16:01","subject":"Re: [PATCH v4 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 Wed, Dec 11, 2024 at 7:06 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Tue, Dec 10, 2024 at 02:23:58PM +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 | 40 ++++++++++++++++++++++++++++-------\n> > 1 file changed, 32 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f3f570544..3b10f207e 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1182,6 +1182,18 @@ void CameraDevice::requestComplete(Request *request)\n> > * post-processing/compression fails.\n> > */\n> > for (auto &buffer : descriptor->buffers_) {\n> > + for (auto &[_, frameBuffer] : request->buffers()) {\n>\n> I've been thinking about ways to avoid the double lookup here, we\n> don't have that many requests or descriptor, but avoiding doing it for every\n> request might be desirable.\n>\n> I've been trying to do so by setting a pointer to a StreamBuffer as\n> libcamera::FrameBuffer::cookie and retrieve it here. However, as\n> now multiple Android streams can map to the same FrameBuffer there's\n> no 1-to-1 association anymore between a StreamBuffer and a\n> FrameBuffer...\n>\n> Too bad, I would have liked to avoid this\n\nYeah, unless we introduce new member variables, it's hard to fix it here.\n\nI don't think we need to worry about this too much, as the upcoming\nbufferCompleted only allows source buffers to be completed one by one.\n\n>\n> > + if (buffer.srcBuffer != frameBuffer &&\n> > + buffer.frameBuffer.get() != frameBuffer)\n> > + continue;\n> > +\n> > + StreamBuffer::Status status = StreamBuffer::Status::Success;\n> > + if (frameBuffer->metadata().status != FrameMetadata::FrameSuccess) {\n> > + status = StreamBuffer::Status::Error;\n> > + }\n>\n> No {} for single line statements\n\nDone\n\n>\n> > + setBufferStatus(buffer, status);\n> > + }\n> > +\n> > CameraStream *stream = buffer.stream;\n>\n> Can you move this after the comment block if you have to resend ?\n\nSure\n\n>\n> >\n> > /*\n> > @@ -1198,22 +1210,18 @@ void CameraDevice::requestComplete(Request *request)\n> > if (fence)\n> > buffer.fence = fence->release();\n> > }\n> > - buffer.status = StreamBuffer::Status::Success;\n> > }\n> >\n> > /*\n> > - * If the Request has failed, abort the request by notifying the error\n> > - * and complete the request with all buffers in error state.\n> > + * If the Request has failed, complete the request with all buffers in\n> > + * error state and notify an error result.\n>\n> With this patch buffers are completed indivdually in error state just\n> below. I would move the below if() before inspecting the single\n> buffers or even drop it completely and just rely on the above loop, after\n> all if one buffer has failed the whole request has failed and you now\n> set its state to Camera3RequestDescriptor::Status::Error in the above\n> call to setBufferStatus()\n\nYeah the comment doesn't make sense anymore. Let's remove it.\n\n>\n> > */\n> > if (request->status() != Request::RequestComplete) {\n> > LOG(HAL, Error) << \"Request \" << request->cookie()\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> > @@ -1238,7 +1246,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> > @@ -1271,7 +1279,13 @@ void CameraDevice::requestComplete(Request *request)\n> > continue;\n> > }\n> >\n> > + if (buffer->status == StreamBuffer::Status::Error) {\n> > + iter = descriptor->pendingStreamsToProcess_.erase(iter);\n> > + continue;\n> > + }\n>\n> nit: could you move this before the previous if() ? There's no point\n> in searching the FrameBuffer if we know the stream has failed ?\n\nRight, done.\n\n>\n> > +\n> > ++iter;\n> > +\n> > int ret = stream->process(buffer);\n> > if (ret) {\n> > setBufferStatus(*buffer, StreamBuffer::Status::Error);\n> > @@ -1324,6 +1338,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>\n> metdata and buffers ?\n\nSure, updated.\n\n>\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>\n> I would drop \"since ...\"\n\nDone\n\nBR,\nHarvey\n\n>\n> All minors, next one should be good to go\n>\n> Thanks\n> j\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 D2ED7BD80A\n\tfor ;\n\tThu, 12 Dec 2024 09:16:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC23067EBE;\n\tThu, 12 Dec 2024 10:16:14 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8C8067E6D\n\tfor ;\n\tThu, 12 Dec 2024 10:16:12 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-30227ccf803so3679251fa.2\n\tfor ;\n\tThu, 12 Dec 2024 01:16:12 -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=\"S2xyWR0q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733994972; x=1734599772;\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=v2xI8xuJIguK1efkoYPD3yQbtF+cKJ1NCtDXrs/f5PA=;\n\tb=S2xyWR0qAH0wTd4pmXRhSGIqrGhG3uTAHVQ/PWd1RGPz2f3PX3acbPPmbIcViq8AB6\n\t+i4JnrqzBwLJCRAVbRjYzJhUy04i7FslXYdFMqM152Sr/IMxT7jeOm0z75MVHyuwZqdn\n\tSBc8WUXV7lAvaxpB/xv7wNJZpbZisg607hkyU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733994972; x=1734599772;\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=v2xI8xuJIguK1efkoYPD3yQbtF+cKJ1NCtDXrs/f5PA=;\n\tb=T5EgpvOI6B0QTu7RHKKa4QkuiXIl3gl9kSZPXTjQbvAGrYne4RuRrASVu5yimKlm1M\n\tpGbxIabP+GFXjIStha6oPEF2w/n9cKWUGZSdR1QEK2iRMGSy969u331MPI7e/PBpebNd\n\tZI0pMxUO+v1RabhEfPpGIM9wx8gqZkAIpqhituwNUJrJK+OsJQm06f3OR9FVLo1dlGm+\n\tCui+z8g5hV74SwPQUdekWvssFpdPz5jjs8lZLox6x1f5Lng+gtRHMWu0weSidGXVY7f3\n\t4WXIKl9PY2Y1gTsbXqf9BqtrgxbRPFWtUJPxf7x+RVDjseFcFIS0e5fl23cjT8x3e4w1\n\tIjqw==","X-Gm-Message-State":"AOJu0YzfwBVuqaABPHzDpguL6PYP99Cl9h9EO82AP1Y7gUdJQjfSD663\n\tCfFEu8yTjXK7xnLoE3lfa/HVKq5npA2YdsAh91Gdm2gaKWe8Qp3/sLPRrWVQq2tKOrg4yVHJ842\n\tzSKYbjSCYb3KefJiSxia6b7fvxIW1EF0vebWo","X-Gm-Gg":"ASbGncvOaOw+EG5I1SUqrWwKIkukTQOiUdwH1J3z1BEciwy1+BpQiy8uGo6Ozpj1vvk\n\tAq+jpTDENWI6FCq5bHHYSsLjkl+oRLVS/t/+owo18E4eEGirZC8ScmFw1/Fsorx48PQ==","X-Google-Smtp-Source":"AGHT+IEHMcZnZSbWeNGPFxKUSnsoabBlEZQG33hJID6dt1IxLGMh3t7fRRlTs7cgFy7BM+cKEGgk7d4eosdKpf9A3C8=","X-Received":"by 2002:a05:651c:221e:b0:302:3356:7ce2 with SMTP id\n\t38308e7fff4ca-3024a0cd92fmr7637011fa.11.1733994971932;\n\tThu, 12 Dec 2024 01:16:11 -0800 (PST)","MIME-Version":"1.0","References":"<20241210142557.2886315-1-chenghaoyang@chromium.org>\n\t<20241210142557.2886315-6-chenghaoyang@chromium.org>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 12 Dec 2024 17:16:01 +0800","Message-ID":"","Subject":"Re: [PATCH v4 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\" "}}]