| Message ID | 20211018132923.476242-4-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The camera3_capture_result_t is only needed to convey capture results to > the camera service through the process_capture_result() callback. > There's no need to store it in the Camera3RequestDescriptor. Build it > dynamically in CameraDevice::sendCaptureResults() instead. > > This requires storing the result metadata created in > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side > effect of this change is that the request metadata lifetime will match > the Camera3RequestDescriptor instead of being destroyed at the end of > requestComplete(). This will be needed to support asynchronous > post-processing, where the request completion will be signaled to the > camera service asynchronously from requestComplete(). > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 43 ++++++++++++++++------------------- > src/android/camera_request.h | 2 +- > 2 files changed, 21 insertions(+), 24 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b4ab5da1..c6ae8930 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > { > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > - camera3_capture_result_t &result = descriptor->captureResult_; > - result.num_output_buffers = descriptor->buffers_.size(); > - result.frame_number = descriptor->frameNumber_; > - result.partial_result = 0; > - > - std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); > - for (auto [i, buffer] : utils::enumerate(resultBuffers)) { > - buffer = descriptor->buffers_[i]; > - buffer.release_fence = descriptor->buffers_[i].acquire_fence; > + for (auto &buffer : descriptor->buffers_) { > + buffer.release_fence = buffer.acquire_fence; > buffer.acquire_fence = -1; > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > } > - result.output_buffers = resultBuffers.data(); > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > } > @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request) > * The buffer status is set to OK and later changed to ERROR if > * post-processing/compression fails. > */ > - camera3_capture_result_t &captureResult = descriptor->captureResult_; > - captureResult.frame_number = descriptor->frameNumber_; > - captureResult.num_output_buffers = descriptor->buffers_.size(); > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > CameraStream *cameraStream = > static_cast<CameraStream *>(buffer.stream->priv); > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request) > buffer.release_fence = -1; > buffer.status = CAMERA3_BUFFER_STATUS_OK; > } > - captureResult.output_buffers = descriptor->buffers_.data(); > - captureResult.partial_result = 1; > > /* > * If the Request has failed, abort the request by notifying the error > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request) > notifyError(descriptor->frameNumber_, nullptr, > CAMERA3_MSG_ERROR_REQUEST); > > - captureResult.partial_result = 0; > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > /* > * Signal to the framework it has to handle fences that > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request) > * Notify if the metadata generation has failed, but continue processing > * buffers and return an empty metadata pack. > */ > - std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor); > - if (!resultMetadata) { > + descriptor->resultMetadata_ = getResultMetadata(*descriptor); > + if (!descriptor->resultMetadata_) { > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > /* The camera framework expects an empty metadata pack on error. */ > - resultMetadata = std::make_unique<CameraMetadata>(0, 0); > + descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0); Since we don't sendCaptureResult() and return here we get to post-processing with a (0, 0) metadata pack. I wonder if the post-processing code is robust enough to cope with that situation (even if I suspect I already know the answer :) It is my understanding the situation is like this already, so it's fine, but maybe we should add a todo. > } > > /* Handle post-processing. */ > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request) > > int ret = cameraStream->process(*src, buffer, > descriptor->settings_, > - resultMetadata.get()); > + descriptor->resultMetadata_.get()); > /* > * Return the FrameBuffer to the CameraStream now that we're > * done processing it. > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request) > } > } > > - captureResult.result = resultMetadata->get(); > descriptor->status_ = Camera3RequestDescriptor::Status::Success; > sendCaptureResults(); > } > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults() > * impact on performance which should be measured. > */ > lock.unlock(); > - callbacks_->process_capture_result(callbacks_, > - &descriptor->captureResult_); > + > + camera3_capture_result_t captureResult = {}; > + > + captureResult.frame_number = descriptor->frameNumber_; > + if (descriptor->resultMetadata_) Do we need this ? As resultMetadata_ is a unique_ptr<> it is constructed owning nothing, and calling get() on it simnply return nullptr, which I think it's what we want. Thanks j > + captureResult.result = descriptor->resultMetadata_->get(); > + captureResult.num_output_buffers = descriptor->buffers_.size(); > + captureResult.output_buffers = descriptor->buffers_.data(); > + > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Success) > + captureResult.partial_result = 1; > + > + callbacks_->process_capture_result(callbacks_, &captureResult); > + > lock.lock(); > } > } > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > index 79dfdb58..db13f624 100644 > --- a/src/android/camera_request.h > +++ b/src/android/camera_request.h > @@ -40,8 +40,8 @@ public: > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > CameraMetadata settings_; > std::unique_ptr<CaptureRequest> request_; > + std::unique_ptr<CameraMetadata> resultMetadata_; > > - camera3_capture_result_t captureResult_ = {}; > Status status_ = Status::Pending; > > private: > -- > 2.31.0 >
Hi Jacopo On 10/18/21 9:18 PM, Jacopo Mondi wrote: > Hi Umang, > > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote: >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> The camera3_capture_result_t is only needed to convey capture results to >> the camera service through the process_capture_result() callback. >> There's no need to store it in the Camera3RequestDescriptor. Build it >> dynamically in CameraDevice::sendCaptureResults() instead. >> >> This requires storing the result metadata created in >> CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side >> effect of this change is that the request metadata lifetime will match >> the Camera3RequestDescriptor instead of being destroyed at the end of >> requestComplete(). This will be needed to support asynchronous >> post-processing, where the request completion will be signaled to the >> camera service asynchronously from requestComplete(). >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 43 ++++++++++++++++------------------- >> src/android/camera_request.h | 2 +- >> 2 files changed, 21 insertions(+), 24 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index b4ab5da1..c6ae8930 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const >> { >> notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); >> >> - camera3_capture_result_t &result = descriptor->captureResult_; >> - result.num_output_buffers = descriptor->buffers_.size(); >> - result.frame_number = descriptor->frameNumber_; >> - result.partial_result = 0; >> - >> - std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); >> - for (auto [i, buffer] : utils::enumerate(resultBuffers)) { >> - buffer = descriptor->buffers_[i]; >> - buffer.release_fence = descriptor->buffers_[i].acquire_fence; >> + for (auto &buffer : descriptor->buffers_) { >> + buffer.release_fence = buffer.acquire_fence; >> buffer.acquire_fence = -1; >> buffer.status = CAMERA3_BUFFER_STATUS_ERROR; >> } >> - result.output_buffers = resultBuffers.data(); >> >> descriptor->status_ = Camera3RequestDescriptor::Status::Error; >> } >> @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request) >> * The buffer status is set to OK and later changed to ERROR if >> * post-processing/compression fails. >> */ >> - camera3_capture_result_t &captureResult = descriptor->captureResult_; >> - captureResult.frame_number = descriptor->frameNumber_; >> - captureResult.num_output_buffers = descriptor->buffers_.size(); >> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { >> CameraStream *cameraStream = >> static_cast<CameraStream *>(buffer.stream->priv); >> @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request) >> buffer.release_fence = -1; >> buffer.status = CAMERA3_BUFFER_STATUS_OK; >> } >> - captureResult.output_buffers = descriptor->buffers_.data(); >> - captureResult.partial_result = 1; >> >> /* >> * If the Request has failed, abort the request by notifying the error >> @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request) >> notifyError(descriptor->frameNumber_, nullptr, >> CAMERA3_MSG_ERROR_REQUEST); >> >> - captureResult.partial_result = 0; >> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { >> /* >> * Signal to the framework it has to handle fences that >> @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request) >> * Notify if the metadata generation has failed, but continue processing >> * buffers and return an empty metadata pack. >> */ >> - std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor); >> - if (!resultMetadata) { >> + descriptor->resultMetadata_ = getResultMetadata(*descriptor); >> + if (!descriptor->resultMetadata_) { >> notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); >> >> /* The camera framework expects an empty metadata pack on error. */ >> - resultMetadata = std::make_unique<CameraMetadata>(0, 0); >> + descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0); > Since we don't sendCaptureResult() and return here we get to > post-processing with a (0, 0) metadata pack. I wonder if the > post-processing code is robust enough to cope with that situation > (even if I suspect I already know the answer :) > > It is my understanding the situation is like this already, so it's > fine, but maybe we should add a todo. \todo stating what? The situation or the solution. I am not sure of a potential solution of this yet > >> } >> >> /* Handle post-processing. */ >> @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request) >> >> int ret = cameraStream->process(*src, buffer, >> descriptor->settings_, >> - resultMetadata.get()); >> + descriptor->resultMetadata_.get()); >> /* >> * Return the FrameBuffer to the CameraStream now that we're >> * done processing it. >> @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request) >> } >> } >> >> - captureResult.result = resultMetadata->get(); >> descriptor->status_ = Camera3RequestDescriptor::Status::Success; >> sendCaptureResults(); >> } >> @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults() >> * impact on performance which should be measured. >> */ >> lock.unlock(); >> - callbacks_->process_capture_result(callbacks_, >> - &descriptor->captureResult_); >> + >> + camera3_capture_result_t captureResult = {}; >> + >> + captureResult.frame_number = descriptor->frameNumber_; >> + if (descriptor->resultMetadata_) > Do we need this ? As resultMetadata_ is a unique_ptr<> it is > constructed owning nothing, and calling get() on it simnply return > nullptr, which I think it's what we want. Yes we need this. There are two get() here a) one of the unique_ptr one, .get() b) another is the descriptor->resultMetadata_->get()) one If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it, will segfault So I found best to guard it with an if () block for now. Does it make sense? > > Thanks > j >> + captureResult.result = descriptor->resultMetadata_->get(); >> + captureResult.num_output_buffers = descriptor->buffers_.size(); >> + captureResult.output_buffers = descriptor->buffers_.data(); >> + >> + if (descriptor->status_ == Camera3RequestDescriptor::Status::Success) >> + captureResult.partial_result = 1; >> + >> + callbacks_->process_capture_result(callbacks_, &captureResult); >> + >> lock.lock(); >> } >> } >> diff --git a/src/android/camera_request.h b/src/android/camera_request.h >> index 79dfdb58..db13f624 100644 >> --- a/src/android/camera_request.h >> +++ b/src/android/camera_request.h >> @@ -40,8 +40,8 @@ public: >> std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; >> CameraMetadata settings_; >> std::unique_ptr<CaptureRequest> request_; >> + std::unique_ptr<CameraMetadata> resultMetadata_; >> >> - camera3_capture_result_t captureResult_ = {}; >> Status status_ = Status::Pending; >> >> private: >> -- >> 2.31.0 >>
Hi Umang, On Mon, Oct 18, 2021 at 09:42:24PM +0530, Umang Jain wrote: > Hi Jacopo > > On 10/18/21 9:18 PM, Jacopo Mondi wrote: > > Hi Umang, > > > > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > The camera3_capture_result_t is only needed to convey capture results to > > > the camera service through the process_capture_result() callback. > > > There's no need to store it in the Camera3RequestDescriptor. Build it > > > dynamically in CameraDevice::sendCaptureResults() instead. > > > > > > This requires storing the result metadata created in > > > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side > > > effect of this change is that the request metadata lifetime will match > > > the Camera3RequestDescriptor instead of being destroyed at the end of > > > requestComplete(). This will be needed to support asynchronous > > > post-processing, where the request completion will be signaled to the > > > camera service asynchronously from requestComplete(). > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > src/android/camera_device.cpp | 43 ++++++++++++++++------------------- > > > src/android/camera_request.h | 2 +- > > > 2 files changed, 21 insertions(+), 24 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index b4ab5da1..c6ae8930 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > > > { > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > > > - camera3_capture_result_t &result = descriptor->captureResult_; > > > - result.num_output_buffers = descriptor->buffers_.size(); > > > - result.frame_number = descriptor->frameNumber_; > > > - result.partial_result = 0; > > > - > > > - std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); > > > - for (auto [i, buffer] : utils::enumerate(resultBuffers)) { > > > - buffer = descriptor->buffers_[i]; > > > - buffer.release_fence = descriptor->buffers_[i].acquire_fence; > > > + for (auto &buffer : descriptor->buffers_) { > > > + buffer.release_fence = buffer.acquire_fence; > > > buffer.acquire_fence = -1; > > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > } > > > - result.output_buffers = resultBuffers.data(); > > > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > } > > > @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request) > > > * The buffer status is set to OK and later changed to ERROR if > > > * post-processing/compression fails. > > > */ > > > - camera3_capture_result_t &captureResult = descriptor->captureResult_; > > > - captureResult.frame_number = descriptor->frameNumber_; > > > - captureResult.num_output_buffers = descriptor->buffers_.size(); > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > > CameraStream *cameraStream = > > > static_cast<CameraStream *>(buffer.stream->priv); > > > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request) > > > buffer.release_fence = -1; > > > buffer.status = CAMERA3_BUFFER_STATUS_OK; > > > } > > > - captureResult.output_buffers = descriptor->buffers_.data(); > > > - captureResult.partial_result = 1; > > > > > > /* > > > * If the Request has failed, abort the request by notifying the error > > > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request) > > > notifyError(descriptor->frameNumber_, nullptr, > > > CAMERA3_MSG_ERROR_REQUEST); > > > > > > - captureResult.partial_result = 0; > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > > /* > > > * Signal to the framework it has to handle fences that > > > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request) > > > * Notify if the metadata generation has failed, but continue processing > > > * buffers and return an empty metadata pack. > > > */ > > > - std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor); > > > - if (!resultMetadata) { > > > + descriptor->resultMetadata_ = getResultMetadata(*descriptor); > > > + if (!descriptor->resultMetadata_) { > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > > > > > /* The camera framework expects an empty metadata pack on error. */ > > > - resultMetadata = std::make_unique<CameraMetadata>(0, 0); > > > + descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0); > > Since we don't sendCaptureResult() and return here we get to > > post-processing with a (0, 0) metadata pack. I wonder if the > > post-processing code is robust enough to cope with that situation > > (even if I suspect I already know the answer :) > > > > It is my understanding the situation is like this already, so it's > > fine, but maybe we should add a todo. > > > \todo stating what? The situation or the solution. I am not sure of a > potential solution of this yet > I wish I had a solution. Just recording the potential issue, if you think it's opportune. > > > > > } > > > > > > /* Handle post-processing. */ > > > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request) > > > > > > int ret = cameraStream->process(*src, buffer, > > > descriptor->settings_, > > > - resultMetadata.get()); > > > + descriptor->resultMetadata_.get()); > > > /* > > > * Return the FrameBuffer to the CameraStream now that we're > > > * done processing it. > > > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request) > > > } > > > } > > > > > > - captureResult.result = resultMetadata->get(); > > > descriptor->status_ = Camera3RequestDescriptor::Status::Success; > > > sendCaptureResults(); > > > } > > > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults() > > > * impact on performance which should be measured. > > > */ > > > lock.unlock(); > > > - callbacks_->process_capture_result(callbacks_, > > > - &descriptor->captureResult_); > > > + > > > + camera3_capture_result_t captureResult = {}; > > > + > > > + captureResult.frame_number = descriptor->frameNumber_; > > > + if (descriptor->resultMetadata_) > > Do we need this ? As resultMetadata_ is a unique_ptr<> it is > > constructed owning nothing, and calling get() on it simnply return > > nullptr, which I think it's what we want. > > > Yes we need this. There are two get() here > > a) one of the unique_ptr one, .get() > > b) another is the descriptor->resultMetadata_->get()) one > > If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it, > will segfault > > So I found best to guard it with an if () block for now. Does it make sense? > Sorry, I confused unique_ptr<>.get() with CameraMetadata::get(). > > > > Thanks > > j > > > + captureResult.result = descriptor->resultMetadata_->get(); > > > + captureResult.num_output_buffers = descriptor->buffers_.size(); > > > + captureResult.output_buffers = descriptor->buffers_.data(); > > > + > > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Success) > > > + captureResult.partial_result = 1; > > > + > > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > > + > > > lock.lock(); > > > } > > > } > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > > index 79dfdb58..db13f624 100644 > > > --- a/src/android/camera_request.h > > > +++ b/src/android/camera_request.h > > > @@ -40,8 +40,8 @@ public: > > > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > > > CameraMetadata settings_; > > > std::unique_ptr<CaptureRequest> request_; > > > + std::unique_ptr<CameraMetadata> resultMetadata_; > > > > > > - camera3_capture_result_t captureResult_ = {}; > > > Status status_ = Status::Pending; > > > > > > private: > > > -- > > > 2.31.0 > > >
On Mon, Oct 18, 2021 at 06:42:51PM +0200, Jacopo Mondi wrote: > Hi Umang, > > On Mon, Oct 18, 2021 at 09:42:24PM +0530, Umang Jain wrote: > > Hi Jacopo > > > > On 10/18/21 9:18 PM, Jacopo Mondi wrote: > > > Hi Umang, > > > > > > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote: > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > The camera3_capture_result_t is only needed to convey capture results to > > > > the camera service through the process_capture_result() callback. > > > > There's no need to store it in the Camera3RequestDescriptor. Build it > > > > dynamically in CameraDevice::sendCaptureResults() instead. > > > > > > > > This requires storing the result metadata created in > > > > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side > > > > effect of this change is that the request metadata lifetime will match > > > > the Camera3RequestDescriptor instead of being destroyed at the end of > > > > requestComplete(). This will be needed to support asynchronous > > > > post-processing, where the request completion will be signaled to the > > > > camera service asynchronously from requestComplete(). > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > --- > > > > src/android/camera_device.cpp | 43 ++++++++++++++++------------------- > > > > src/android/camera_request.h | 2 +- > > > > 2 files changed, 21 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index b4ab5da1..c6ae8930 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > > > > { > > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > > > > > - camera3_capture_result_t &result = descriptor->captureResult_; > > > > - result.num_output_buffers = descriptor->buffers_.size(); > > > > - result.frame_number = descriptor->frameNumber_; > > > > - result.partial_result = 0; > > > > - > > > > - std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); > > > > - for (auto [i, buffer] : utils::enumerate(resultBuffers)) { > > > > - buffer = descriptor->buffers_[i]; > > > > - buffer.release_fence = descriptor->buffers_[i].acquire_fence; > > > > + for (auto &buffer : descriptor->buffers_) { > > > > + buffer.release_fence = buffer.acquire_fence; > > > > buffer.acquire_fence = -1; > > > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > > } > > > > - result.output_buffers = resultBuffers.data(); > > > > > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > > } > > > > @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request) > > > > * The buffer status is set to OK and later changed to ERROR if > > > > * post-processing/compression fails. > > > > */ > > > > - camera3_capture_result_t &captureResult = descriptor->captureResult_; > > > > - captureResult.frame_number = descriptor->frameNumber_; > > > > - captureResult.num_output_buffers = descriptor->buffers_.size(); > > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > > > CameraStream *cameraStream = > > > > static_cast<CameraStream *>(buffer.stream->priv); > > > > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request) > > > > buffer.release_fence = -1; > > > > buffer.status = CAMERA3_BUFFER_STATUS_OK; > > > > } > > > > - captureResult.output_buffers = descriptor->buffers_.data(); > > > > - captureResult.partial_result = 1; > > > > > > > > /* > > > > * If the Request has failed, abort the request by notifying the error > > > > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request) > > > > notifyError(descriptor->frameNumber_, nullptr, > > > > CAMERA3_MSG_ERROR_REQUEST); > > > > > > > > - captureResult.partial_result = 0; > > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > > > /* > > > > * Signal to the framework it has to handle fences that > > > > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request) > > > > * Notify if the metadata generation has failed, but continue processing > > > > * buffers and return an empty metadata pack. > > > > */ > > > > - std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor); > > > > - if (!resultMetadata) { > > > > + descriptor->resultMetadata_ = getResultMetadata(*descriptor); > > > > + if (!descriptor->resultMetadata_) { > > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > > > > > > > /* The camera framework expects an empty metadata pack on error. */ > > > > - resultMetadata = std::make_unique<CameraMetadata>(0, 0); > > > > + descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0); > > > > > > Since we don't sendCaptureResult() and return here we get to > > > post-processing with a (0, 0) metadata pack. I wonder if the > > > post-processing code is robust enough to cope with that situation > > > (even if I suspect I already know the answer :) > > > > > > It is my understanding the situation is like this already, so it's > > > fine, but maybe we should add a todo. > > > > \todo stating what? The situation or the solution. I am not sure of a > > potential solution of this yet > > I wish I had a solution. Just recording the potential issue, if you > think it's opportune. * \todo Check that the post-processor code handles this situation * correctly. > > > > } > > > > > > > > /* Handle post-processing. */ > > > > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request) > > > > > > > > int ret = cameraStream->process(*src, buffer, > > > > descriptor->settings_, > > > > - resultMetadata.get()); > > > > + descriptor->resultMetadata_.get()); > > > > /* > > > > * Return the FrameBuffer to the CameraStream now that we're > > > > * done processing it. > > > > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request) > > > > } > > > > } > > > > > > > > - captureResult.result = resultMetadata->get(); > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Success; > > > > sendCaptureResults(); > > > > } > > > > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults() > > > > * impact on performance which should be measured. > > > > */ > > > > lock.unlock(); > > > > - callbacks_->process_capture_result(callbacks_, > > > > - &descriptor->captureResult_); > > > > + > > > > + camera3_capture_result_t captureResult = {}; > > > > + > > > > + captureResult.frame_number = descriptor->frameNumber_; > > > > + if (descriptor->resultMetadata_) > > > > > > Do we need this ? As resultMetadata_ is a unique_ptr<> it is > > > constructed owning nothing, and calling get() on it simnply return > > > nullptr, which I think it's what we want. > > > > Yes we need this. There are two get() here > > > > a) one of the unique_ptr one, .get() > > > > b) another is the descriptor->resultMetadata_->get()) one > > > > If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it, > > will segfault > > > > So I found best to guard it with an if () block for now. Does it make sense? > > Sorry, I confused unique_ptr<>.get() with CameraMetadata::get(). Renaming CameraMetadata::get() to getMetadata() may be a good idea to avoid future confusion. > > > > + captureResult.result = descriptor->resultMetadata_->get(); > > > > + captureResult.num_output_buffers = descriptor->buffers_.size(); > > > > + captureResult.output_buffers = descriptor->buffers_.data(); > > > > + > > > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Success) > > > > + captureResult.partial_result = 1; > > > > + > > > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > > > + > > > > lock.lock(); > > > > } > > > > } > > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > > > index 79dfdb58..db13f624 100644 > > > > --- a/src/android/camera_request.h > > > > +++ b/src/android/camera_request.h > > > > @@ -40,8 +40,8 @@ public: > > > > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > > > > CameraMetadata settings_; > > > > std::unique_ptr<CaptureRequest> request_; > > > > + std::unique_ptr<CameraMetadata> resultMetadata_; > > > > > > > > - camera3_capture_result_t captureResult_ = {}; > > > > Status status_ = Status::Pending; > > > > > > > > private:
Hi Umang and Laurent, thank you for the patch. On Tue, Oct 19, 2021 at 2:39 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Oct 18, 2021 at 06:42:51PM +0200, Jacopo Mondi wrote: > > Hi Umang, > > > > On Mon, Oct 18, 2021 at 09:42:24PM +0530, Umang Jain wrote: > > > Hi Jacopo > > > > > > On 10/18/21 9:18 PM, Jacopo Mondi wrote: > > > > Hi Umang, > > > > > > > > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote: > > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > The camera3_capture_result_t is only needed to convey capture results to > > > > > the camera service through the process_capture_result() callback. > > > > > There's no need to store it in the Camera3RequestDescriptor. Build it > > > > > dynamically in CameraDevice::sendCaptureResults() instead. > > > > > > > > > > This requires storing the result metadata created in > > > > > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side > > > > > effect of this change is that the request metadata lifetime will match > > > > > the Camera3RequestDescriptor instead of being destroyed at the end of > > > > > requestComplete(). This will be needed to support asynchronous > > > > > post-processing, where the request completion will be signaled to the > > > > > camera service asynchronously from requestComplete(). > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > --- > > > > > src/android/camera_device.cpp | 43 ++++++++++++++++------------------- > > > > > src/android/camera_request.h | 2 +- > > > > > 2 files changed, 21 insertions(+), 24 deletions(-) > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > index b4ab5da1..c6ae8930 100644 > > > > > --- a/src/android/camera_device.cpp > > > > > +++ b/src/android/camera_device.cpp > > > > > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const > > > > > { > > > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); > > > > > > > > > > - camera3_capture_result_t &result = descriptor->captureResult_; > > > > > - result.num_output_buffers = descriptor->buffers_.size(); > > > > > - result.frame_number = descriptor->frameNumber_; > > > > > - result.partial_result = 0; > > > > > - > > > > > - std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); > > > > > - for (auto [i, buffer] : utils::enumerate(resultBuffers)) { > > > > > - buffer = descriptor->buffers_[i]; > > > > > - buffer.release_fence = descriptor->buffers_[i].acquire_fence; > > > > > + for (auto &buffer : descriptor->buffers_) { > > > > > + buffer.release_fence = buffer.acquire_fence; > > > > > buffer.acquire_fence = -1; > > > > > buffer.status = CAMERA3_BUFFER_STATUS_ERROR; > > > > > } > > > > > - result.output_buffers = resultBuffers.data(); > > > > > > > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > > > } > > > > > @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request) > > > > > * The buffer status is set to OK and later changed to ERROR if > > > > > * post-processing/compression fails. > > > > > */ > > > > > - camera3_capture_result_t &captureResult = descriptor->captureResult_; > > > > > - captureResult.frame_number = descriptor->frameNumber_; > > > > > - captureResult.num_output_buffers = descriptor->buffers_.size(); > > > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > > > > CameraStream *cameraStream = > > > > > static_cast<CameraStream *>(buffer.stream->priv); > > > > > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request) > > > > > buffer.release_fence = -1; > > > > > buffer.status = CAMERA3_BUFFER_STATUS_OK; > > > > > } > > > > > - captureResult.output_buffers = descriptor->buffers_.data(); > > > > > - captureResult.partial_result = 1; > > > > > > > > > > /* > > > > > * If the Request has failed, abort the request by notifying the error > > > > > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request) > > > > > notifyError(descriptor->frameNumber_, nullptr, > > > > > CAMERA3_MSG_ERROR_REQUEST); > > > > > > > > > > - captureResult.partial_result = 0; > > > > > for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { > > > > > /* > > > > > * Signal to the framework it has to handle fences that > > > > > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request) > > > > > * Notify if the metadata generation has failed, but continue processing > > > > > * buffers and return an empty metadata pack. > > > > > */ > > > > > - std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor); > > > > > - if (!resultMetadata) { > > > > > + descriptor->resultMetadata_ = getResultMetadata(*descriptor); > > > > > + if (!descriptor->resultMetadata_) { > > > > > notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); > > > > > > > > > > /* The camera framework expects an empty metadata pack on error. */ > > > > > - resultMetadata = std::make_unique<CameraMetadata>(0, 0); > > > > > + descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0); > > > > > > > > Since we don't sendCaptureResult() and return here we get to > > > > post-processing with a (0, 0) metadata pack. I wonder if the > > > > post-processing code is robust enough to cope with that situation > > > > (even if I suspect I already know the answer :) > > > > > > > > It is my understanding the situation is like this already, so it's > > > > fine, but maybe we should add a todo. > > > > > > \todo stating what? The situation or the solution. I am not sure of a > > > potential solution of this yet > > > > I wish I had a solution. Just recording the potential issue, if you > > think it's opportune. > > * \todo Check that the post-processor code handles this situation > * correctly. > > > > > > } > > > > > > > > > > /* Handle post-processing. */ > > > > > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request) > > > > > > > > > > int ret = cameraStream->process(*src, buffer, > > > > > descriptor->settings_, > > > > > - resultMetadata.get()); > > > > > + descriptor->resultMetadata_.get()); > > > > > /* > > > > > * Return the FrameBuffer to the CameraStream now that we're > > > > > * done processing it. > > > > > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request) > > > > > } > > > > > } > > > > > > > > > > - captureResult.result = resultMetadata->get(); > > > > > descriptor->status_ = Camera3RequestDescriptor::Status::Success; > > > > > sendCaptureResults(); > > > > > } > > > > > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults() > > > > > * impact on performance which should be measured. > > > > > */ > > > > > lock.unlock(); > > > > > - callbacks_->process_capture_result(callbacks_, > > > > > - &descriptor->captureResult_); > > > > > + > > > > > + camera3_capture_result_t captureResult = {}; > > > > > + > > > > > + captureResult.frame_number = descriptor->frameNumber_; > > > > > + if (descriptor->resultMetadata_) > > > > > > > > Do we need this ? As resultMetadata_ is a unique_ptr<> it is > > > > constructed owning nothing, and calling get() on it simnply return > > > > nullptr, which I think it's what we want. > > > > > > Yes we need this. There are two get() here > > > > > > a) one of the unique_ptr one, .get() > > > > > > b) another is the descriptor->resultMetadata_->get()) one > > > > > > If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it, > > > will segfault > > > > > > So I found best to guard it with an if () block for now. Does it make sense? > > > > Sorry, I confused unique_ptr<>.get() with CameraMetadata::get(). > > Renaming CameraMetadata::get() to getMetadata() may be a good idea to > avoid future confusion. > > > > > > + captureResult.result = descriptor->resultMetadata_->get(); > > > > > + captureResult.num_output_buffers = descriptor->buffers_.size(); > > > > > + captureResult.output_buffers = descriptor->buffers_.data(); > > > > > + > > > > > + if (descriptor->status_ == Camera3RequestDescriptor::Status::Success) > > > > > + captureResult.partial_result = 1; > > > > > + > > > > > + callbacks_->process_capture_result(callbacks_, &captureResult); > > > > > + > > > > > lock.lock(); > > > > > } > > > > > } > > > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > > > > index 79dfdb58..db13f624 100644 > > > > > --- a/src/android/camera_request.h > > > > > +++ b/src/android/camera_request.h > > > > > @@ -40,8 +40,8 @@ public: > > > > > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; > > > > > CameraMetadata settings_; > > > > > std::unique_ptr<CaptureRequest> request_; > > > > > + std::unique_ptr<CameraMetadata> resultMetadata_; > > > > > > > > > > - camera3_capture_result_t captureResult_ = {}; > > > > > Status status_ = Status::Pending; > > > > > > > > > > private: > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index b4ab5da1..c6ae8930 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const { notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); - camera3_capture_result_t &result = descriptor->captureResult_; - result.num_output_buffers = descriptor->buffers_.size(); - result.frame_number = descriptor->frameNumber_; - result.partial_result = 0; - - std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers); - for (auto [i, buffer] : utils::enumerate(resultBuffers)) { - buffer = descriptor->buffers_[i]; - buffer.release_fence = descriptor->buffers_[i].acquire_fence; + for (auto &buffer : descriptor->buffers_) { + buffer.release_fence = buffer.acquire_fence; buffer.acquire_fence = -1; buffer.status = CAMERA3_BUFFER_STATUS_ERROR; } - result.output_buffers = resultBuffers.data(); descriptor->status_ = Camera3RequestDescriptor::Status::Error; } @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request) * The buffer status is set to OK and later changed to ERROR if * post-processing/compression fails. */ - camera3_capture_result_t &captureResult = descriptor->captureResult_; - captureResult.frame_number = descriptor->frameNumber_; - captureResult.num_output_buffers = descriptor->buffers_.size(); for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { CameraStream *cameraStream = static_cast<CameraStream *>(buffer.stream->priv); @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request) buffer.release_fence = -1; buffer.status = CAMERA3_BUFFER_STATUS_OK; } - captureResult.output_buffers = descriptor->buffers_.data(); - captureResult.partial_result = 1; /* * If the Request has failed, abort the request by notifying the error @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request) notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST); - captureResult.partial_result = 0; for (camera3_stream_buffer_t &buffer : descriptor->buffers_) { /* * Signal to the framework it has to handle fences that @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request) * Notify if the metadata generation has failed, but continue processing * buffers and return an empty metadata pack. */ - std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor); - if (!resultMetadata) { + descriptor->resultMetadata_ = getResultMetadata(*descriptor); + if (!descriptor->resultMetadata_) { notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT); /* The camera framework expects an empty metadata pack on error. */ - resultMetadata = std::make_unique<CameraMetadata>(0, 0); + descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0); } /* Handle post-processing. */ @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request) int ret = cameraStream->process(*src, buffer, descriptor->settings_, - resultMetadata.get()); + descriptor->resultMetadata_.get()); /* * Return the FrameBuffer to the CameraStream now that we're * done processing it. @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request) } } - captureResult.result = resultMetadata->get(); descriptor->status_ = Camera3RequestDescriptor::Status::Success; sendCaptureResults(); } @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults() * impact on performance which should be measured. */ lock.unlock(); - callbacks_->process_capture_result(callbacks_, - &descriptor->captureResult_); + + camera3_capture_result_t captureResult = {}; + + captureResult.frame_number = descriptor->frameNumber_; + if (descriptor->resultMetadata_) + captureResult.result = descriptor->resultMetadata_->get(); + captureResult.num_output_buffers = descriptor->buffers_.size(); + captureResult.output_buffers = descriptor->buffers_.data(); + + if (descriptor->status_ == Camera3RequestDescriptor::Status::Success) + captureResult.partial_result = 1; + + callbacks_->process_capture_result(callbacks_, &captureResult); + lock.lock(); } } diff --git a/src/android/camera_request.h b/src/android/camera_request.h index 79dfdb58..db13f624 100644 --- a/src/android/camera_request.h +++ b/src/android/camera_request.h @@ -40,8 +40,8 @@ public: std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_; CameraMetadata settings_; std::unique_ptr<CaptureRequest> request_; + std::unique_ptr<CameraMetadata> resultMetadata_; - camera3_capture_result_t captureResult_ = {}; Status status_ = Status::Pending; private: