| Message ID | 20241210142557.2886315-6-chenghaoyang@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey On Tue, Dec 10, 2024 at 02:23:58PM +0000, Harvey Yang wrote: > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used > for requests that have not done any processing. When a request is > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead. > > To avoid code duplication, when CameraMetadata cannot be generated, > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > src/android/camera_device.cpp | 40 ++++++++++++++++++++++++++++------- > 1 file changed, 32 insertions(+), 8 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index f3f570544..3b10f207e 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1182,6 +1182,18 @@ void CameraDevice::requestComplete(Request *request) > * post-processing/compression fails. > */ > for (auto &buffer : descriptor->buffers_) { > + for (auto &[_, frameBuffer] : request->buffers()) { I've been thinking about ways to avoid the double lookup here, we don't have that many requests or descriptor, but avoiding doing it for every request might be desirable. I've been trying to do so by setting a pointer to a StreamBuffer as libcamera::FrameBuffer::cookie and retrieve it here. However, as now multiple Android streams can map to the same FrameBuffer there's no 1-to-1 association anymore between a StreamBuffer and a FrameBuffer... Too bad, I would have liked to avoid this > + if (buffer.srcBuffer != frameBuffer && > + buffer.frameBuffer.get() != frameBuffer) > + continue; > + > + StreamBuffer::Status status = StreamBuffer::Status::Success; > + if (frameBuffer->metadata().status != FrameMetadata::FrameSuccess) { > + status = StreamBuffer::Status::Error; > + } No {} for single line statements > + setBufferStatus(buffer, status); > + } > + > CameraStream *stream = buffer.stream; Can you move this after the comment block if you have to resend ? > > /* > @@ -1198,22 +1210,18 @@ void CameraDevice::requestComplete(Request *request) > if (fence) > buffer.fence = fence->release(); > } > - buffer.status = StreamBuffer::Status::Success; > } > > /* > - * If the Request has failed, abort the request by notifying the error > - * and complete the request with all buffers in error state. > + * If the Request has failed, complete the request with all buffers in > + * error state and notify an error result. With this patch buffers are completed indivdually in error state just below. I would move the below if() before inspecting the single buffers or even drop it completely and just rely on the above loop, after all if one buffer has failed the whole request has failed and you now set its state to Camera3RequestDescriptor::Status::Error in the above call to setBufferStatus() > */ > if (request->status() != Request::RequestComplete) { > LOG(HAL, Error) << "Request " << request->cookie() > << " not successfully completed: " > << request->status(); > > - abortRequest(descriptor); > - completeDescriptor(descriptor); > - > - return; > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > } > > /* > @@ -1238,7 +1246,7 @@ void CameraDevice::requestComplete(Request *request) > */ > descriptor->resultMetadata_ = getResultMetadata(*descriptor); > if (!descriptor->resultMetadata_) { > - notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > /* > * The camera framework expects an empty metadata pack on error. > @@ -1271,7 +1279,13 @@ void CameraDevice::requestComplete(Request *request) > continue; > } > > + if (buffer->status == StreamBuffer::Status::Error) { > + iter = descriptor->pendingStreamsToProcess_.erase(iter); > + continue; > + } nit: could you move this before the previous if() ? There's no point in searching the FrameBuffer if we know the stream has failed ? > + > ++iter; > + > int ret = stream->process(buffer); > if (ret) { > setBufferStatus(*buffer, StreamBuffer::Status::Error); > @@ -1324,6 +1338,16 @@ void CameraDevice::sendCaptureResults() > descriptors_.pop(); > > sendCaptureResult(descriptor.get()); > + > + /* > + * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some > + * of the expected result metadata might not be available metdata and buffers ? > + * because the capture is cancelled by the camera. Only notify > + * it when the final result is sent, since Android will ignore > + * the following metadata. > + */ I would drop "since ..." All minors, next one should be good to go Thanks j > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Error) > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > } > } > > -- > 2.47.0.338.g60cca15819-goog >
Hi Jacopo, On Wed, Dec 11, 2024 at 7:06 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Tue, Dec 10, 2024 at 02:23:58PM +0000, Harvey Yang wrote: > > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used > > for requests that have not done any processing. When a request is > > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead. > > > > To avoid code duplication, when CameraMetadata cannot be generated, > > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > src/android/camera_device.cpp | 40 ++++++++++++++++++++++++++++------- > > 1 file changed, 32 insertions(+), 8 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index f3f570544..3b10f207e 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1182,6 +1182,18 @@ void CameraDevice::requestComplete(Request *request) > > * post-processing/compression fails. > > */ > > for (auto &buffer : descriptor->buffers_) { > > + for (auto &[_, frameBuffer] : request->buffers()) { > > I've been thinking about ways to avoid the double lookup here, we > don't have that many requests or descriptor, but avoiding doing it for every > request might be desirable. > > I've been trying to do so by setting a pointer to a StreamBuffer as > libcamera::FrameBuffer::cookie and retrieve it here. However, as > now multiple Android streams can map to the same FrameBuffer there's > no 1-to-1 association anymore between a StreamBuffer and a > FrameBuffer... > > Too bad, I would have liked to avoid this Yeah, unless we introduce new member variables, it's hard to fix it here. I don't think we need to worry about this too much, as the upcoming bufferCompleted only allows source buffers to be completed one by one. > > > + if (buffer.srcBuffer != frameBuffer && > > + buffer.frameBuffer.get() != frameBuffer) > > + continue; > > + > > + StreamBuffer::Status status = StreamBuffer::Status::Success; > > + if (frameBuffer->metadata().status != FrameMetadata::FrameSuccess) { > > + status = StreamBuffer::Status::Error; > > + } > > No {} for single line statements Done > > > + setBufferStatus(buffer, status); > > + } > > + > > CameraStream *stream = buffer.stream; > > Can you move this after the comment block if you have to resend ? Sure > > > > > /* > > @@ -1198,22 +1210,18 @@ void CameraDevice::requestComplete(Request *request) > > if (fence) > > buffer.fence = fence->release(); > > } > > - buffer.status = StreamBuffer::Status::Success; > > } > > > > /* > > - * If the Request has failed, abort the request by notifying the error > > - * and complete the request with all buffers in error state. > > + * If the Request has failed, complete the request with all buffers in > > + * error state and notify an error result. > > With this patch buffers are completed indivdually in error state just > below. I would move the below if() before inspecting the single > buffers or even drop it completely and just rely on the above loop, after > all if one buffer has failed the whole request has failed and you now > set its state to Camera3RequestDescriptor::Status::Error in the above > call to setBufferStatus() Yeah the comment doesn't make sense anymore. Let's remove it. > > > */ > > if (request->status() != Request::RequestComplete) { > > LOG(HAL, Error) << "Request " << request->cookie() > > << " not successfully completed: " > > << request->status(); > > > > - abortRequest(descriptor); > > - completeDescriptor(descriptor); > > - > > - return; > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > } > > > > /* > > @@ -1238,7 +1246,7 @@ void CameraDevice::requestComplete(Request *request) > > */ > > descriptor->resultMetadata_ = getResultMetadata(*descriptor); > > if (!descriptor->resultMetadata_) { > > - notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > > /* > > * The camera framework expects an empty metadata pack on error. > > @@ -1271,7 +1279,13 @@ void CameraDevice::requestComplete(Request *request) > > continue; > > } > > > > + if (buffer->status == StreamBuffer::Status::Error) { > > + iter = descriptor->pendingStreamsToProcess_.erase(iter); > > + continue; > > + } > > nit: could you move this before the previous if() ? There's no point > in searching the FrameBuffer if we know the stream has failed ? Right, done. > > > + > > ++iter; > > + > > int ret = stream->process(buffer); > > if (ret) { > > setBufferStatus(*buffer, StreamBuffer::Status::Error); > > @@ -1324,6 +1338,16 @@ void CameraDevice::sendCaptureResults() > > descriptors_.pop(); > > > > sendCaptureResult(descriptor.get()); > > + > > + /* > > + * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some > > + * of the expected result metadata might not be available > > metdata and buffers ? Sure, updated. > > > + * because the capture is cancelled by the camera. Only notify > > + * it when the final result is sent, since Android will ignore > > + * the following metadata. > > + */ > > I would drop "since ..." Done BR, Harvey > > All minors, next one should be good to go > > Thanks > j > > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Error) > > + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > } > > } > > > > -- > > 2.47.0.338.g60cca15819-goog > >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index f3f570544..3b10f207e 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1182,6 +1182,18 @@ void CameraDevice::requestComplete(Request *request) * post-processing/compression fails. */ for (auto &buffer : descriptor->buffers_) { + for (auto &[_, frameBuffer] : request->buffers()) { + if (buffer.srcBuffer != frameBuffer && + buffer.frameBuffer.get() != frameBuffer) + continue; + + StreamBuffer::Status status = StreamBuffer::Status::Success; + if (frameBuffer->metadata().status != FrameMetadata::FrameSuccess) { + status = StreamBuffer::Status::Error; + } + setBufferStatus(buffer, status); + } + CameraStream *stream = buffer.stream; /* @@ -1198,22 +1210,18 @@ void CameraDevice::requestComplete(Request *request) if (fence) buffer.fence = fence->release(); } - buffer.status = StreamBuffer::Status::Success; } /* - * If the Request has failed, abort the request by notifying the error - * and complete the request with all buffers in error state. + * If the Request has failed, complete the request with all buffers in + * error state and notify an error result. */ if (request->status() != Request::RequestComplete) { LOG(HAL, Error) << "Request " << request->cookie() << " not successfully completed: " << request->status(); - abortRequest(descriptor); - completeDescriptor(descriptor); - - return; + descriptor->status_ = Camera3RequestDescriptor::Status::Error; } /* @@ -1238,7 +1246,7 @@ void CameraDevice::requestComplete(Request *request) */ descriptor->resultMetadata_ = getResultMetadata(*descriptor); if (!descriptor->resultMetadata_) { - notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); + descriptor->status_ = Camera3RequestDescriptor::Status::Error; /* * The camera framework expects an empty metadata pack on error. @@ -1271,7 +1279,13 @@ void CameraDevice::requestComplete(Request *request) continue; } + if (buffer->status == StreamBuffer::Status::Error) { + iter = descriptor->pendingStreamsToProcess_.erase(iter); + continue; + } + ++iter; + int ret = stream->process(buffer); if (ret) { setBufferStatus(*buffer, StreamBuffer::Status::Error); @@ -1324,6 +1338,16 @@ void CameraDevice::sendCaptureResults() descriptors_.pop(); sendCaptureResult(descriptor.get()); + + /* + * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some + * of the expected result metadata might not be available + * because the capture is cancelled by the camera. Only notify + * it when the final result is sent, since Android will ignore + * the following metadata. + */ + if (descriptor->status_ == Camera3RequestDescriptor::Status::Error) + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); } }