Message ID | 20241127092632.3145984-7-chenghaoyang@chromium.org |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi Harvey On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote: > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT > should be used instead. > > This patch also cleans up aborting when flushing with > sendCaptureResult(). > > 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 | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 0377cf215..3fb92268e 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > buffer.status = StreamBuffer::Status::Error; > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > + > + sendCaptureResult(descriptor); > } > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > MutexLocker stateLock(stateMutex_); > > if (state_ == State::Flushing) { > - Camera3RequestDescriptor *rawDescriptor = descriptor.get(); > - { > - MutexLocker descriptorsLock(descriptorsMutex_); > - descriptors_.push(std::move(descriptor)); > - } > - abortRequest(rawDescriptor); > - completeDescriptor(rawDescriptor); > - > + abortRequest(descriptor.get()); I'm not sure I get this change ? Specifically why we don't complete the descriptor anymore.. > return 0; > } > > @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request) > << " not successfully completed: " > << request->status(); > > - abortRequest(descriptor); > - completeDescriptor(descriptor); > - > - return; > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > } > > /* > @@ -1239,7 +1231,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. > @@ -1325,6 +1317,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); > } > } > > -- > 2.47.0.338.g60cca15819-goog >
Hi Jacopo, On Thu, Nov 28, 2024 at 11:02 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote: > > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT > > should be used instead. > > > > This patch also cleans up aborting when flushing with > > sendCaptureResult(). > > > > 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 | 28 +++++++++++++++------------- > > 1 file changed, 15 insertions(+), 13 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 0377cf215..3fb92268e 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > > buffer.status = StreamBuffer::Status::Error; > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > + > > + sendCaptureResult(descriptor); > > } > > > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > MutexLocker stateLock(stateMutex_); > > > > if (state_ == State::Flushing) { > > - Camera3RequestDescriptor *rawDescriptor = descriptor.get(); > > - { > > - MutexLocker descriptorsLock(descriptorsMutex_); > > - descriptors_.push(std::move(descriptor)); > > - } > > - abortRequest(rawDescriptor); > > - completeDescriptor(rawDescriptor); > > - > > + abortRequest(descriptor.get()); > > I'm not sure I get this change ? > > Specifically why we don't complete the descriptor anymore.. Okay, apart from the fix of CAMERA3_MSG_ERROR_RESULT instead of CAMERA3_MSG_ERROR_REQUEST, this patch contains one more thing that is only briefly mentioned in the commit message: When flushing and aborting new requests, we should notify CAMERA3_MSG_ERROR_REQUEST, and call `process_capture_result` earlier, instead of waiting for the previous requests being completed first. `completeDescriptor` would trigger the in-order completion of requests only, while in this patch, we call `sendCaptureResult` directly, without the necessity to set `complete_` anymore. Hope it helps explain. BR, Harvey > > > > return 0; > > } > > > > @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request) > > << " not successfully completed: " > > << request->status(); > > > > - abortRequest(descriptor); > > - completeDescriptor(descriptor); > > - > > - return; > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > } > > > > /* > > @@ -1239,7 +1231,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. > > @@ -1325,6 +1317,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); > > } > > } > > > > -- > > 2.47.0.338.g60cca15819-goog > >
Hi Harvey On Thu, Nov 28, 2024 at 11:30:15PM +0800, Cheng-Hao Yang wrote: > Hi Jacopo, > > On Thu, Nov 28, 2024 at 11:02 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Harvey > > > > On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote: > > > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT > > > should be used instead. > > > > > > This patch also cleans up aborting when flushing with > > > sendCaptureResult(). > > > > > > 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 | 28 +++++++++++++++------------- > > > 1 file changed, 15 insertions(+), 13 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 0377cf215..3fb92268e 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > > > buffer.status = StreamBuffer::Status::Error; > > > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > + > > > + sendCaptureResult(descriptor); > > > } > > > > > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > > > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > MutexLocker stateLock(stateMutex_); > > > > > > if (state_ == State::Flushing) { > > > - Camera3RequestDescriptor *rawDescriptor = descriptor.get(); > > > - { > > > - MutexLocker descriptorsLock(descriptorsMutex_); > > > - descriptors_.push(std::move(descriptor)); > > > - } > > > - abortRequest(rawDescriptor); > > > - completeDescriptor(rawDescriptor); > > > - > > > + abortRequest(descriptor.get()); > > > > I'm not sure I get this change ? > > > > Specifically why we don't complete the descriptor anymore.. > > Okay, apart from the fix of CAMERA3_MSG_ERROR_RESULT > instead of CAMERA3_MSG_ERROR_REQUEST, this patch Where did this happen ? In flush ? I read it used to call abortRequest() that notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); and used to call void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor) { MutexLocker lock(descriptorsMutex_); descriptor->complete_ = true; sendCaptureResults(); } So I might have missed what do we gain with this change (as abortRequest() has a single call place if I'm not mistaken. Also, this new version won't set descriptor->complete_ = true; as the previous call to completeDescriptor() did. > contains one more thing that is only briefly mentioned in the > commit message: > > When flushing and aborting new requests, we should notify > CAMERA3_MSG_ERROR_REQUEST, and call > `process_capture_result` earlier, instead of waiting for the > previous requests being completed first. > > `completeDescriptor` would trigger the in-order completion > of requests only, while in this patch, we call > `sendCaptureResult` directly, without the necessity to > set `complete_` anymore. > > Hope it helps explain. Ah yes, thanks, so we can't go through completeDescriptor() because it completes in-order but you need to to go through a direct call to sendCaptureResult(). Thanks, it helps. I would have helped even more if you have added it to the commit message, because this When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead. This patch also cleans up aborting when flushing with sendCaptureResult(). doesn't explain me anything and we had to go through three emails and me trying to understand what you have done. Spending 5 more minutes to write a proper commit message would have avoided that. > > BR, > Harvey > > > > > > > > return 0; > > > } > > > > > > @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request) > > > << " not successfully completed: " > > > << request->status(); > > > > > > - abortRequest(descriptor); > > > - completeDescriptor(descriptor); > > > - > > > - return; > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > } > > > > > > /* > > > @@ -1239,7 +1231,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. > > > @@ -1325,6 +1317,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); Is this part of the patch related to the above ? What happens here ? Why do you delay the call to notifyError() ? > > > } > > > } > > > > > > -- > > > 2.47.0.338.g60cca15819-goog > > >
Hi Jacopo, On Tue, Dec 3, 2024 at 12:17 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Thu, Nov 28, 2024 at 11:30:15PM +0800, Cheng-Hao Yang wrote: > > Hi Jacopo, > > > > On Thu, Nov 28, 2024 at 11:02 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Harvey > > > > > > On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote: > > > > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT > > > > should be used instead. > > > > > > > > This patch also cleans up aborting when flushing with > > > > sendCaptureResult(). > > > > > > > > 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 | 28 +++++++++++++++------------- > > > > 1 file changed, 15 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index 0377cf215..3fb92268e 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > > > > buffer.status = StreamBuffer::Status::Error; > > > > > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > > + > > > > + sendCaptureResult(descriptor); > > > > } > > > > > > > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const > > > > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > MutexLocker stateLock(stateMutex_); > > > > > > > > if (state_ == State::Flushing) { > > > > - Camera3RequestDescriptor *rawDescriptor = descriptor.get(); > > > > - { > > > > - MutexLocker descriptorsLock(descriptorsMutex_); > > > > - descriptors_.push(std::move(descriptor)); > > > > - } > > > > - abortRequest(rawDescriptor); > > > > - completeDescriptor(rawDescriptor); > > > > - > > > > + abortRequest(descriptor.get()); > > > > > > I'm not sure I get this change ? > > > > > > Specifically why we don't complete the descriptor anymore.. > > > > Okay, apart from the fix of CAMERA3_MSG_ERROR_RESULT > > instead of CAMERA3_MSG_ERROR_REQUEST, this patch > > Where did this happen ? In flush ? I read it used to call > abortRequest() that > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > and used to call > > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor) > { > MutexLocker lock(descriptorsMutex_); > descriptor->complete_ = true; > > sendCaptureResults(); > } > > So I might have missed what do we gain with this change (as > abortRequest() has a single call place if I'm not mistaken. > Also, this new version won't set > > descriptor->complete_ = true; > > as the previous call to completeDescriptor() did. Seems that you got most of the ideas, regarding your following comments? Let me know if you still have questions :) > > > contains one more thing that is only briefly mentioned in the > > commit message: > > > > When flushing and aborting new requests, we should notify > > CAMERA3_MSG_ERROR_REQUEST, and call > > `process_capture_result` earlier, instead of waiting for the > > previous requests being completed first. > > > > `completeDescriptor` would trigger the in-order completion > > of requests only, while in this patch, we call > > `sendCaptureResult` directly, without the necessity to > > set `complete_` anymore. > > > > Hope it helps explain. > > Ah yes, thanks, so we can't go through completeDescriptor() because it > completes in-order but you need to to go through a direct call to > sendCaptureResult(). > > Thanks, it helps. I would have helped even more if you have added it > to the commit message, because this > > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT > should be used instead. > > This patch also cleans up aborting when flushing with > sendCaptureResult(). > > doesn't explain me anything and we had to go through three emails and > me trying to understand what you have done. Spending 5 more minutes to > write a proper commit message would have avoided that. Yeah sorry, I'll do better next time... > > > > > BR, > > Harvey > > > > > > > > > > > > return 0; > > > > } > > > > > > > > @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request) > > > > << " not successfully completed: " > > > > << request->status(); > > > > > > > > - abortRequest(descriptor); > > > > - completeDescriptor(descriptor); > > > > - > > > > - return; > > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > > } > > > > > > > > /* > > > > @@ -1239,7 +1231,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. > > > > @@ -1325,6 +1317,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); > > Is this part of the patch related to the above ? What happens here ? > Why do you delay the call to notifyError() ? Actually there are quite some things to discuss here: We don't think the way tot is implemented that notifying CAMERA3_MSG_ERROR_RESULT when `getResultMetadata()` fails make sense, as `getResultMetadata()` only fails when we fail to allocate memory for CameraMetadata IIUC. In the last patch, you can find that this logic is removed. We may also consider removing this logic with an extra patch. This patch calls CAMERA3_MSG_ERROR_RESULT when the libcamera::Request was cancelled, which will happen when any buffer was cancelled [1]. It's quite different from CAMERA3_MSG_ERROR_RESULT's description [2] though, which indicates that there is some/all metadata missing. Currently we don't check the absence of metadata and report an error, yet. My question is: Do you think that simplifying and taking RequestCancelled as CAMERA3_MSG_ERROR_RESULT makes sense? We can also not keep it as CAMERA3_MSG_ERROR_REQUEST, because we're trying to support partial results, and CAMERA3_MSG_ERROR_REQUEST is currently triggered in signal requestCompleted, and it's for "pending requests that have not done any processing". Having completed partial results and CAMERA3_MSG_ERROR_REQUEST doesn't make much sense to me (although according to the description, it's not restricted as well? I'm not sure...) WDYT? BR, Harvey [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/request.cpp#n108 [2]: https://source.android.com/reference/hal/structcamera3__device__ops > > > > > } > > > > } > > > > > > > > -- > > > > 2.47.0.338.g60cca15819-goog > > > >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 0377cf215..3fb92268e 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const buffer.status = StreamBuffer::Status::Error; descriptor->status_ = Camera3RequestDescriptor::Status::Error; + + sendCaptureResult(descriptor); } bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques MutexLocker stateLock(stateMutex_); if (state_ == State::Flushing) { - Camera3RequestDescriptor *rawDescriptor = descriptor.get(); - { - MutexLocker descriptorsLock(descriptorsMutex_); - descriptors_.push(std::move(descriptor)); - } - abortRequest(rawDescriptor); - completeDescriptor(rawDescriptor); - + abortRequest(descriptor.get()); return 0; } @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request) << " not successfully completed: " << request->status(); - abortRequest(descriptor); - completeDescriptor(descriptor); - - return; + descriptor->status_ = Camera3RequestDescriptor::Status::Error; } /* @@ -1239,7 +1231,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. @@ -1325,6 +1317,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); } }