| Message ID | 20210920173752.1346190-10-umang.jain@ideasonboard.com |
|---|---|
| State | RFC |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote: > A Camera3RequestDescriptors is constructed and queued to descriptors_ > queue as soon as an (incoming) capture request is received on the > libcamera HAL. The capture request is picked up by HAL, in order to run > it to completion. At completion, CameraDevice::requestComplete() gets > invoked and capture results are populated and ready to be sent back > to the framework. > > All the data and framebuffers associated with the request are alive > and encapsulated inside this Camera3RequestDescriptor descriptor. > By inspecting the ProcessStatus on the descriptor, we can now send > capture results via the process_capture_result() callback. > > Hence, introduce a new private member function sendCaptureResults() > which will be responsible to send capture results back to the > framework by inspecting the descriptor on the queue. In subsequent > commit, when the post processsor shall run async, sendCaptureResults() > can be called from the post-processor's thread for e.g. > streamProcessComplete() hence, introduce the mutex lock to avoid the > races. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 47 +++++++++++++++++++++++++---------- > src/android/camera_device.h | 4 +++ > 2 files changed, 38 insertions(+), 13 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 4658e881..16ecdfc5 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1078,7 +1078,7 @@ 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 = {}; > + 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_) { > @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request) > continue; > } > > + if (cameraStream->type() == CameraStream::Type::Internal) > + descriptor->internalBuffer_ = src; > + > descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; > > cameraStream->process(src, *buffer.buffer, descriptor); > - > - /* > - * Return the FrameBuffer to the CameraStream now that we're > - * done processing it. > - */ > - if (cameraStream->type() == CameraStream::Type::Internal) > - cameraStream->putBuffer(src); > + return; > } > > - captureResult.result = descriptor->resultMetadata_->get(); > - callbacks_->process_capture_result(callbacks_, &captureResult); > - > - descriptors_.pop_front(); > + /* > + * Mark the status on the descriptor as success as no processing > + * is neeeded. > + */ > + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > + sendCaptureResults(); > } > > > -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, > +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, > Camera3RequestDescriptor *request) > { > if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) { > @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer > CAMERA3_MSG_ERROR_BUFFER); > } > } > + > + /* > + * Return the FrameBuffer to the CameraStream now that we're > + * done processing it. > + */ > + if (cameraStream->type() == CameraStream::Type::Internal) > + cameraStream->putBuffer(request->internalBuffer_); > + > + sendCaptureResults(); > +} > + > +void CameraDevice::sendCaptureResults() > +{ > + Camera3RequestDescriptor *d = descriptors_.front().get(); If I'm not mistaken, in case a request that doesn't need post-processing will complete while the post-processed one is still being worked on, this could lead to a completion order inversion processCaptureRequest(n) descriptors.push(n) processCaptureRequest(n+1) descriptors.push(n+1) equestComplete(n) postProcess(n) -----------> process(n) | .... ... | requestComplete(n+1) | sendCaptureResult(n+1) | d = descriptors.front() | process_capture_res(n) | | sendCaptureResult(n) <---------------- d = descritpros.front() process_capture_res(n + 1) > + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || > + d->status_ == Camera3RequestDescriptor::ProcessStatus::None) > + return; > + > + MutexLocker lock(descriptorsMutex_); > + d->captureResult_.result = d->resultMetadata_->get(); > + callbacks_->process_capture_result(callbacks_, &(d->captureResult_)); > + descriptors_.pop_front(); > } > > std::string CameraDevice::logPrefix() const > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 60c134dc..0bd570a1 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor { > std::unique_ptr<CaptureRequest> request_; > std::unique_ptr<CameraMetadata> resultMetadata_; > > + camera3_capture_result_t captureResult_ = {}; > + libcamera::FrameBuffer *internalBuffer_; > + > ProcessStatus status_; > }; > > @@ -118,6 +121,7 @@ private: > int processControls(Camera3RequestDescriptor *descriptor); > std::unique_ptr<CameraMetadata> getResultMetadata( > const Camera3RequestDescriptor *descriptor) const; > + void sendCaptureResults(); > > unsigned int id_; > camera3_device_t camera3Device_; > -- > 2.31.1 >
Hi Umang, On Tue, Sep 21, 2021 at 01:46:18PM +0200, Jacopo Mondi wrote: > Hi Umang, > > On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote: > > A Camera3RequestDescriptors is constructed and queued to descriptors_ > > queue as soon as an (incoming) capture request is received on the > > libcamera HAL. The capture request is picked up by HAL, in order to run > > it to completion. At completion, CameraDevice::requestComplete() gets > > invoked and capture results are populated and ready to be sent back > > to the framework. > > > > All the data and framebuffers associated with the request are alive > > and encapsulated inside this Camera3RequestDescriptor descriptor. > > By inspecting the ProcessStatus on the descriptor, we can now send > > capture results via the process_capture_result() callback. > > > > Hence, introduce a new private member function sendCaptureResults() > > which will be responsible to send capture results back to the > > framework by inspecting the descriptor on the queue. In subsequent > > commit, when the post processsor shall run async, sendCaptureResults() > > can be called from the post-processor's thread for e.g. > > streamProcessComplete() hence, introduce the mutex lock to avoid the > > races. > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 47 +++++++++++++++++++++++++---------- > > src/android/camera_device.h | 4 +++ > > 2 files changed, 38 insertions(+), 13 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 4658e881..16ecdfc5 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1078,7 +1078,7 @@ 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 = {}; > > + 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_) { > > @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request) > > continue; > > } > > > > + if (cameraStream->type() == CameraStream::Type::Internal) > > + descriptor->internalBuffer_ = src; > > + > > descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; > > > > cameraStream->process(src, *buffer.buffer, descriptor); > > - > > - /* > > - * Return the FrameBuffer to the CameraStream now that we're > > - * done processing it. > > - */ > > - if (cameraStream->type() == CameraStream::Type::Internal) > > - cameraStream->putBuffer(src); > > + return; > > } > > > > - captureResult.result = descriptor->resultMetadata_->get(); > > - callbacks_->process_capture_result(callbacks_, &captureResult); > > - > > - descriptors_.pop_front(); > > + /* > > + * Mark the status on the descriptor as success as no processing > > + * is neeeded. > > + */ > > + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > > + sendCaptureResults(); > > } > > > > > > -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, > > +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, > > Camera3RequestDescriptor *request) > > { > > if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) { > > @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer > > CAMERA3_MSG_ERROR_BUFFER); > > } > > } > > + > > + /* > > + * Return the FrameBuffer to the CameraStream now that we're > > + * done processing it. > > + */ > > + if (cameraStream->type() == CameraStream::Type::Internal) > > + cameraStream->putBuffer(request->internalBuffer_); > > + > > + sendCaptureResults(); > > +} > > + > > +void CameraDevice::sendCaptureResults() > > +{ > > + Camera3RequestDescriptor *d = descriptors_.front().get(); > > If I'm not mistaken, in case a request that doesn't need > post-processing will complete while the post-processed one is still > being worked on, this could lead to a completion order inversion > > processCaptureRequest(n) > descriptors.push(n) > > processCaptureRequest(n+1) > descriptors.push(n+1) > > equestComplete(n) > postProcess(n) -----------> process(n) > | > .... ... > | > requestComplete(n+1) | > sendCaptureResult(n+1) | > d = descriptors.front() | > process_capture_res(n) | > | > sendCaptureResult(n) <---------------- > d = descritpros.front() > process_capture_res(n + 1) as you have clarified offline this can't happen as you skip not-completed requests in sendCaptureResult(). At the same time it seems you need to iterate the queue when the front() request has completed, to complete the next ones which are ready but whose completion was post-poned. Thanks j > > > + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || > > + d->status_ == Camera3RequestDescriptor::ProcessStatus::None) > > + return; > > + > > + MutexLocker lock(descriptorsMutex_); > > + d->captureResult_.result = d->resultMetadata_->get(); > > + callbacks_->process_capture_result(callbacks_, &(d->captureResult_)); > > + descriptors_.pop_front(); > > } > > > > std::string CameraDevice::logPrefix() const > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 60c134dc..0bd570a1 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor { > > std::unique_ptr<CaptureRequest> request_; > > std::unique_ptr<CameraMetadata> resultMetadata_; > > > > + camera3_capture_result_t captureResult_ = {}; > > + libcamera::FrameBuffer *internalBuffer_; > > + > > ProcessStatus status_; > > }; > > > > @@ -118,6 +121,7 @@ private: > > int processControls(Camera3RequestDescriptor *descriptor); > > std::unique_ptr<CameraMetadata> getResultMetadata( > > const Camera3RequestDescriptor *descriptor) const; > > + void sendCaptureResults(); > > > > unsigned int id_; > > camera3_device_t camera3Device_; > > -- > > 2.31.1 > >
Hi Jacopo, (Post in call discussion summary on this if anyone here is interested) On 9/21/21 5:16 PM, Jacopo Mondi wrote: > Hi Umang, > > On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote: >> A Camera3RequestDescriptors is constructed and queued to descriptors_ >> queue as soon as an (incoming) capture request is received on the >> libcamera HAL. The capture request is picked up by HAL, in order to run >> it to completion. At completion, CameraDevice::requestComplete() gets >> invoked and capture results are populated and ready to be sent back >> to the framework. >> >> All the data and framebuffers associated with the request are alive >> and encapsulated inside this Camera3RequestDescriptor descriptor. >> By inspecting the ProcessStatus on the descriptor, we can now send >> capture results via the process_capture_result() callback. >> >> Hence, introduce a new private member function sendCaptureResults() >> which will be responsible to send capture results back to the >> framework by inspecting the descriptor on the queue. In subsequent >> commit, when the post processsor shall run async, sendCaptureResults() >> can be called from the post-processor's thread for e.g. >> streamProcessComplete() hence, introduce the mutex lock to avoid the >> races. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++---------- >> src/android/camera_device.h | 4 +++ >> 2 files changed, 38 insertions(+), 13 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 4658e881..16ecdfc5 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -1078,7 +1078,7 @@ 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 = {}; >> + 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_) { >> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request) >> continue; >> } >> >> + if (cameraStream->type() == CameraStream::Type::Internal) >> + descriptor->internalBuffer_ = src; >> + >> descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; >> >> cameraStream->process(src, *buffer.buffer, descriptor); >> - >> - /* >> - * Return the FrameBuffer to the CameraStream now that we're >> - * done processing it. >> - */ >> - if (cameraStream->type() == CameraStream::Type::Internal) >> - cameraStream->putBuffer(src); >> + return; >> } >> >> - captureResult.result = descriptor->resultMetadata_->get(); >> - callbacks_->process_capture_result(callbacks_, &captureResult); >> - >> - descriptors_.pop_front(); >> + /* >> + * Mark the status on the descriptor as success as no processing >> + * is neeeded. >> + */ >> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success; >> + sendCaptureResults(); >> } >> >> >> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, >> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, >> Camera3RequestDescriptor *request) >> { >> if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) { >> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer >> CAMERA3_MSG_ERROR_BUFFER); >> } >> } >> + >> + /* >> + * Return the FrameBuffer to the CameraStream now that we're >> + * done processing it. >> + */ >> + if (cameraStream->type() == CameraStream::Type::Internal) >> + cameraStream->putBuffer(request->internalBuffer_); >> + >> + sendCaptureResults(); >> +} >> + >> +void CameraDevice::sendCaptureResults() >> +{ >> + Camera3RequestDescriptor *d = descriptors_.front().get(); > If I'm not mistaken, in case a request that doesn't need > post-processing will complete while the post-processed one is still > being worked on, this could lead to a completion order inversion > > processCaptureRequest(n) > descriptors.push(n) > > processCaptureRequest(n+1) > descriptors.push(n+1) > > equestComplete(n) > postProcess(n) -----------> process(n) > | > .... ... > | > requestComplete(n+1) | > sendCaptureResult(n+1) | > d = descriptors.front() | > process_capture_res(n) | > | > sendCaptureResult(n) <---------------- > d = descritpros.front() > process_capture_res(n + 1) > Nice flow diagram btw, I need to get better at that ;-) So, first of all sendCaptureResults() doesn't take an argument, but I get your point that you are trying to match for which 'n' request, sendCapture result is called... sendCaptureResults() works on the queue's head. It doesn't care from where it's called; it can be called from requestComplete()[same thread] or streamProcessingComplete()[different thread]. All it does or suppose to do, is basically looks at the queue's head and see if the HEAD descriptor says - "Send me back to the framework". - If yes, it will send the results via process_capture_result of the HEAD and drop the HEAD from the queue - If no, simply return; That's it. Since, it looks at the head, it will only send results in order (since it's a queue) and drop the HEAD. However, the discussion has un-covered another point that sendCaptureResults() can lag behind w.r.t libcamera:Requests being completed, since there is no loop around to process other descriptors in the queue which are ready to send (these descriptors can potentially be the ones not requiring any post-processing). As discussed in the call, we both agree that we should probably iterate over the queue and send the results back to the framework as soon as possible, for ready descriptor. So, sendCaptureResults() will be wrapped in a loop in subsequent version. >> + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || >> + d->status_ == Camera3RequestDescriptor::ProcessStatus::None) >> + return; >> + >> + MutexLocker lock(descriptorsMutex_); >> + d->captureResult_.result = d->resultMetadata_->get(); >> + callbacks_->process_capture_result(callbacks_, &(d->captureResult_)); >> + descriptors_.pop_front(); >> } >> >> std::string CameraDevice::logPrefix() const >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >> index 60c134dc..0bd570a1 100644 >> --- a/src/android/camera_device.h >> +++ b/src/android/camera_device.h >> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor { >> std::unique_ptr<CaptureRequest> request_; >> std::unique_ptr<CameraMetadata> resultMetadata_; >> >> + camera3_capture_result_t captureResult_ = {}; >> + libcamera::FrameBuffer *internalBuffer_; >> + >> ProcessStatus status_; >> }; >> >> @@ -118,6 +121,7 @@ private: >> int processControls(Camera3RequestDescriptor *descriptor); >> std::unique_ptr<CameraMetadata> getResultMetadata( >> const Camera3RequestDescriptor *descriptor) const; >> + void sendCaptureResults(); >> >> unsigned int id_; >> camera3_device_t camera3Device_; >> -- >> 2.31.1 >>
Hi Umang, Thank you for the patch. On Tue, Sep 21, 2021 at 06:25:03PM +0530, Umang Jain wrote: > Hi Jacopo, > > (Post in call discussion summary on this if anyone here is interested) > > On 9/21/21 5:16 PM, Jacopo Mondi wrote: > > On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote: > >> A Camera3RequestDescriptors is constructed and queued to descriptors_ s/Camera3RequestDescriptors/Camera3RequestDescriptor/ > >> queue as soon as an (incoming) capture request is received on the > >> libcamera HAL. The capture request is picked up by HAL, in order to run > >> it to completion. At completion, CameraDevice::requestComplete() gets > >> invoked and capture results are populated and ready to be sent back > >> to the framework. > >> > >> All the data and framebuffers associated with the request are alive > >> and encapsulated inside this Camera3RequestDescriptor descriptor. > >> By inspecting the ProcessStatus on the descriptor, we can now send > >> capture results via the process_capture_result() callback. > >> > >> Hence, introduce a new private member function sendCaptureResults() > >> which will be responsible to send capture results back to the > >> framework by inspecting the descriptor on the queue. In subsequent > >> commit, when the post processsor shall run async, sendCaptureResults() > >> can be called from the post-processor's thread for e.g. > >> streamProcessComplete() hence, introduce the mutex lock to avoid the > >> races. > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++---------- > >> src/android/camera_device.h | 4 +++ > >> 2 files changed, 38 insertions(+), 13 deletions(-) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index 4658e881..16ecdfc5 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -1078,7 +1078,7 @@ 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 = {}; > >> + 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_) { > >> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request) > >> continue; > >> } > >> > >> + if (cameraStream->type() == CameraStream::Type::Internal) > >> + descriptor->internalBuffer_ = src; > >> + > >> descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; > >> > >> cameraStream->process(src, *buffer.buffer, descriptor); > >> - > >> - /* > >> - * Return the FrameBuffer to the CameraStream now that we're > >> - * done processing it. > >> - */ > >> - if (cameraStream->type() == CameraStream::Type::Internal) > >> - cameraStream->putBuffer(src); > >> + return; > >> } > >> > >> - captureResult.result = descriptor->resultMetadata_->get(); > >> - callbacks_->process_capture_result(callbacks_, &captureResult); > >> - > >> - descriptors_.pop_front(); > >> + /* > >> + * Mark the status on the descriptor as success as no processing > >> + * is neeeded. > >> + */ > >> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > >> + sendCaptureResults(); > >> } > >> > >> > >> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, > >> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, > >> Camera3RequestDescriptor *request) > >> { > >> if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) { > >> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer > >> CAMERA3_MSG_ERROR_BUFFER); > >> } > >> } > >> + > >> + /* > >> + * Return the FrameBuffer to the CameraStream now that we're > >> + * done processing it. > >> + */ > >> + if (cameraStream->type() == CameraStream::Type::Internal) > >> + cameraStream->putBuffer(request->internalBuffer_); > >> + > >> + sendCaptureResults(); > >> +} > >> + > >> +void CameraDevice::sendCaptureResults() > >> +{ > >> + Camera3RequestDescriptor *d = descriptors_.front().get(); The variable is named descriptor everywhere else, let's keep a common naming scheme (I'm tempted to rename Camera3RequestDescriptor to Camera3Request, and thus also rename all variables accordingly, but that's for later, on top of this series). > > > > If I'm not mistaken, in case a request that doesn't need > > post-processing will complete while the post-processed one is still > > being worked on, this could lead to a completion order inversion > > > > processCaptureRequest(n) > > descriptors.push(n) > > > > processCaptureRequest(n+1) > > descriptors.push(n+1) > > > > equestComplete(n) > > postProcess(n) -----------> process(n) > > | > > .... ... > > | > > requestComplete(n+1) | > > sendCaptureResult(n+1) | > > d = descriptors.front() | > > process_capture_res(n) | > > | > > sendCaptureResult(n) <---------------- > > d = descritpros.front() > > process_capture_res(n + 1) > > > > Nice flow diagram btw, I need to get better at that ;-) > > So, first of all sendCaptureResults() doesn't take an argument, but I > get your point that you are trying to match for which 'n' request, > sendCapture result is called... > > sendCaptureResults() works on the queue's head. It doesn't care from > where it's called; it can be called from requestComplete()[same thread] > or streamProcessingComplete()[different thread]. All it does or suppose > to do, is basically looks at the queue's head and see if the HEAD > descriptor says - "Send me back to the framework". > - If yes, it will send the results via process_capture_result of the > HEAD and drop the HEAD from the queue > - If no, simply return; > > That's it. Since, it looks at the head, it will only send results in > order (since it's a queue) and drop the HEAD. > > However, the discussion has un-covered another point that > sendCaptureResults() can lag behind w.r.t libcamera:Requests being > completed, since there is no loop around to process other descriptors in > the queue which are ready to send (these descriptors can potentially be > the ones not requiring any post-processing). As discussed in the call, > we both agree that we should probably iterate over the queue and send > the results back to the framework as soon as possible, for ready > descriptor. So, sendCaptureResults() will be wrapped in a loop in > subsequent version. I'd include the loop inside sendCaptureResults(). Something along the lines of MutexLocker lock(descriptorsMutex_); while (!descriptors_.empty() && descriptors_.front()->status_ != Camera3RequestDescriptor::Status::Pending) { std::unique_ptr<Camera3RequestDescriptor> descriptor = std::move(descriptors_.front()); descriptors_.pop(); lock.unlock(); /* Signal completion to the camera service. */ lock.lock(); } (see my review of 07/10 for the Pending status) Note that you'll need to handle both the Success and Error statuses here, as requests that complete with an error need to be completed in order too. One a side note, maybe adding the following helper to Camera3RequestDescriptor would be nice, to make the loop more readable: bool isPending() const { return status_ == Status::Pending; } You would then get while (!descriptors_.empty() && descriptors_.front()->isPending()) { ... } > >> + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || > >> + d->status_ == Camera3RequestDescriptor::ProcessStatus::None) > >> + return; > >> + > >> + MutexLocker lock(descriptorsMutex_); > >> + d->captureResult_.result = d->resultMetadata_->get(); > >> + callbacks_->process_capture_result(callbacks_, &(d->captureResult_)); > >> + descriptors_.pop_front(); > >> } > >> > >> std::string CameraDevice::logPrefix() const > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > >> index 60c134dc..0bd570a1 100644 > >> --- a/src/android/camera_device.h > >> +++ b/src/android/camera_device.h > >> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor { > >> std::unique_ptr<CaptureRequest> request_; > >> std::unique_ptr<CameraMetadata> resultMetadata_; > >> > >> + camera3_capture_result_t captureResult_ = {}; > >> + libcamera::FrameBuffer *internalBuffer_; > >> + > >> ProcessStatus status_; > >> }; > >> > >> @@ -118,6 +121,7 @@ private: > >> int processControls(Camera3RequestDescriptor *descriptor); > >> std::unique_ptr<CameraMetadata> getResultMetadata( > >> const Camera3RequestDescriptor *descriptor) const; > >> + void sendCaptureResults(); > >> > >> unsigned int id_; > >> camera3_device_t camera3Device_;
Hi Umang, thank you for the patch. On Wed, Sep 22, 2021 at 8:27 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Umang, > > Thank you for the patch. > > On Tue, Sep 21, 2021 at 06:25:03PM +0530, Umang Jain wrote: > > Hi Jacopo, > > > > (Post in call discussion summary on this if anyone here is interested) > > > > On 9/21/21 5:16 PM, Jacopo Mondi wrote: > > > On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote: > > >> A Camera3RequestDescriptors is constructed and queued to descriptors_ > > s/Camera3RequestDescriptors/Camera3RequestDescriptor/ > > > >> queue as soon as an (incoming) capture request is received on the > > >> libcamera HAL. The capture request is picked up by HAL, in order to run > > >> it to completion. At completion, CameraDevice::requestComplete() gets > > >> invoked and capture results are populated and ready to be sent back > > >> to the framework. > > >> > > >> All the data and framebuffers associated with the request are alive > > >> and encapsulated inside this Camera3RequestDescriptor descriptor. > > >> By inspecting the ProcessStatus on the descriptor, we can now send > > >> capture results via the process_capture_result() callback. > > >> > > >> Hence, introduce a new private member function sendCaptureResults() > > >> which will be responsible to send capture results back to the > > >> framework by inspecting the descriptor on the queue. In subsequent > > >> commit, when the post processsor shall run async, sendCaptureResults() > > >> can be called from the post-processor's thread for e.g. > > >> streamProcessComplete() hence, introduce the mutex lock to avoid the > > >> races. > > >> > > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > >> --- > > >> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++---------- > > >> src/android/camera_device.h | 4 +++ > > >> 2 files changed, 38 insertions(+), 13 deletions(-) > > >> > > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > >> index 4658e881..16ecdfc5 100644 > > >> --- a/src/android/camera_device.cpp > > >> +++ b/src/android/camera_device.cpp > > >> @@ -1078,7 +1078,7 @@ 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 = {}; > > >> + 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_) { > > >> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request) > > >> continue; > > >> } > > >> > > >> + if (cameraStream->type() == CameraStream::Type::Internal) > > >> + descriptor->internalBuffer_ = src; > > >> + > > >> descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; > > >> > > >> cameraStream->process(src, *buffer.buffer, descriptor); > > >> - > > >> - /* > > >> - * Return the FrameBuffer to the CameraStream now that we're > > >> - * done processing it. > > >> - */ > > >> - if (cameraStream->type() == CameraStream::Type::Internal) > > >> - cameraStream->putBuffer(src); > > >> + return; I read the patches 07-09/10. The new code looks to assume the number of buffers that require post processing is only one. I consider so from 1.) return added here 2.) no protection to the Status 3.) no check to wait for the number of pot-processing in streamProcessingComplete(), so that a capture request complete is sent multiple times. I also think descriptors_ pop logic is wrong. I think we ideally deny a request given in requestComplete if it is not a top of descriptors_. The top of descriptors is kept to the currently processing capture request. I think we should have two queues here like following. descriptors_ - for request processed by camera descriptors_on_processing - for request processed by post processors Then we pop descriptors_ in completeRequest and descriptors_on_processing. Even if a capture request doesn't require post processing, we should add it to descriptors_on_processing to send it after processing for all the preceding request is complete These may be resolved in the next patch. But this happens at least at this patch if I understand correctly. -Hiro > > >> } > > >> > > >> - captureResult.result = descriptor->resultMetadata_->get(); > > >> - callbacks_->process_capture_result(callbacks_, &captureResult); > > >> - > > >> - descriptors_.pop_front(); > > >> + /* > > >> + * Mark the status on the descriptor as success as no processing > > >> + * is neeeded. > > >> + */ > > >> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > > >> + sendCaptureResults(); > > >> } > > >> > > >> > > >> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, > > >> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, > > >> Camera3RequestDescriptor *request) > > >> { > > >> if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) { > > >> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer > > >> CAMERA3_MSG_ERROR_BUFFER); > > >> } > > >> } > > >> + > > >> + /* > > >> + * Return the FrameBuffer to the CameraStream now that we're > > >> + * done processing it. > > >> + */ > > >> + if (cameraStream->type() == CameraStream::Type::Internal) > > >> + cameraStream->putBuffer(request->internalBuffer_); > > >> + > > >> + sendCaptureResults(); > > >> +} > > >> + > > >> +void CameraDevice::sendCaptureResults() > > >> +{ > > >> + Camera3RequestDescriptor *d = descriptors_.front().get(); > > The variable is named descriptor everywhere else, let's keep a common > naming scheme (I'm tempted to rename Camera3RequestDescriptor to > Camera3Request, and thus also rename all variables accordingly, but > that's for later, on top of this series). > > > > > > > If I'm not mistaken, in case a request that doesn't need > > > post-processing will complete while the post-processed one is still > > > being worked on, this could lead to a completion order inversion > > > > > > processCaptureRequest(n) > > > descriptors.push(n) > > > > > > processCaptureRequest(n+1) > > > descriptors.push(n+1) > > > > > > equestComplete(n) > > > postProcess(n) -----------> process(n) > > > | > > > .... ... > > > | > > > requestComplete(n+1) | > > > sendCaptureResult(n+1) | > > > d = descriptors.front() | > > > process_capture_res(n) | > > > | > > > sendCaptureResult(n) <---------------- > > > d = descritpros.front() > > > process_capture_res(n + 1) > > > > > > > Nice flow diagram btw, I need to get better at that ;-) > > > > So, first of all sendCaptureResults() doesn't take an argument, but I > > get your point that you are trying to match for which 'n' request, > > sendCapture result is called... > > > > sendCaptureResults() works on the queue's head. It doesn't care from > > where it's called; it can be called from requestComplete()[same thread] > > or streamProcessingComplete()[different thread]. All it does or suppose > > to do, is basically looks at the queue's head and see if the HEAD > > descriptor says - "Send me back to the framework". > > - If yes, it will send the results via process_capture_result of the > > HEAD and drop the HEAD from the queue > > - If no, simply return; > > > > That's it. Since, it looks at the head, it will only send results in > > order (since it's a queue) and drop the HEAD. > > > > However, the discussion has un-covered another point that > > sendCaptureResults() can lag behind w.r.t libcamera:Requests being > > completed, since there is no loop around to process other descriptors in > > the queue which are ready to send (these descriptors can potentially be > > the ones not requiring any post-processing). As discussed in the call, > > we both agree that we should probably iterate over the queue and send > > the results back to the framework as soon as possible, for ready > > descriptor. So, sendCaptureResults() will be wrapped in a loop in > > subsequent version. > > I'd include the loop inside sendCaptureResults(). Something along the > lines of > > MutexLocker lock(descriptorsMutex_); > > while (!descriptors_.empty() && > descriptors_.front()->status_ != Camera3RequestDescriptor::Status::Pending) { > std::unique_ptr<Camera3RequestDescriptor> descriptor = > std::move(descriptors_.front()); > descriptors_.pop(); > > lock.unlock(); > > /* Signal completion to the camera service. */ > > lock.lock(); > } > > (see my review of 07/10 for the Pending status) > > Note that you'll need to handle both the Success and Error statuses > here, as requests that complete with an error need to be completed in > order too. > > One a side note, maybe adding the following helper to > Camera3RequestDescriptor would be nice, to make the loop more readable: > > bool isPending() const { return status_ == Status::Pending; } > > You would then get > > while (!descriptors_.empty() && descriptors_.front()->isPending()) { > ... > } > > > >> + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || > > >> + d->status_ == Camera3RequestDescriptor::ProcessStatus::None) > > >> + return; > > >> + > > >> + MutexLocker lock(descriptorsMutex_); > > >> + d->captureResult_.result = d->resultMetadata_->get(); > > >> + callbacks_->process_capture_result(callbacks_, &(d->captureResult_)); > > >> + descriptors_.pop_front(); > > >> } > > >> > > >> std::string CameraDevice::logPrefix() const > > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > >> index 60c134dc..0bd570a1 100644 > > >> --- a/src/android/camera_device.h > > >> +++ b/src/android/camera_device.h > > >> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor { > > >> std::unique_ptr<CaptureRequest> request_; > > >> std::unique_ptr<CameraMetadata> resultMetadata_; > > >> > > >> + camera3_capture_result_t captureResult_ = {}; > > >> + libcamera::FrameBuffer *internalBuffer_; > > >> + > > >> ProcessStatus status_; > > >> }; > > >> > > >> @@ -118,6 +121,7 @@ private: > > >> int processControls(Camera3RequestDescriptor *descriptor); > > >> std::unique_ptr<CameraMetadata> getResultMetadata( > > >> const Camera3RequestDescriptor *descriptor) const; > > >> + void sendCaptureResults(); > > >> > > >> unsigned int id_; > > >> camera3_device_t camera3Device_; > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On 9/27/21 12:32 PM, Hirokazu Honda wrote: > Hi Umang, thank you for the patch. > > On Wed, Sep 22, 2021 at 8:27 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> Hi Umang, >> >> Thank you for the patch. >> >> On Tue, Sep 21, 2021 at 06:25:03PM +0530, Umang Jain wrote: >>> Hi Jacopo, >>> >>> (Post in call discussion summary on this if anyone here is interested) >>> >>> On 9/21/21 5:16 PM, Jacopo Mondi wrote: >>>> On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote: >>>>> A Camera3RequestDescriptors is constructed and queued to descriptors_ >> s/Camera3RequestDescriptors/Camera3RequestDescriptor/ >> >>>>> queue as soon as an (incoming) capture request is received on the >>>>> libcamera HAL. The capture request is picked up by HAL, in order to run >>>>> it to completion. At completion, CameraDevice::requestComplete() gets >>>>> invoked and capture results are populated and ready to be sent back >>>>> to the framework. >>>>> >>>>> All the data and framebuffers associated with the request are alive >>>>> and encapsulated inside this Camera3RequestDescriptor descriptor. >>>>> By inspecting the ProcessStatus on the descriptor, we can now send >>>>> capture results via the process_capture_result() callback. >>>>> >>>>> Hence, introduce a new private member function sendCaptureResults() >>>>> which will be responsible to send capture results back to the >>>>> framework by inspecting the descriptor on the queue. In subsequent >>>>> commit, when the post processsor shall run async, sendCaptureResults() >>>>> can be called from the post-processor's thread for e.g. >>>>> streamProcessComplete() hence, introduce the mutex lock to avoid the >>>>> races. >>>>> >>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>> --- >>>>> src/android/camera_device.cpp | 47 +++++++++++++++++++++++++---------- >>>>> src/android/camera_device.h | 4 +++ >>>>> 2 files changed, 38 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>>> index 4658e881..16ecdfc5 100644 >>>>> --- a/src/android/camera_device.cpp >>>>> +++ b/src/android/camera_device.cpp >>>>> @@ -1078,7 +1078,7 @@ 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 = {}; >>>>> + 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_) { >>>>> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request) >>>>> continue; >>>>> } >>>>> >>>>> + if (cameraStream->type() == CameraStream::Type::Internal) >>>>> + descriptor->internalBuffer_ = src; >>>>> + >>>>> descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; >>>>> >>>>> cameraStream->process(src, *buffer.buffer, descriptor); >>>>> - >>>>> - /* >>>>> - * Return the FrameBuffer to the CameraStream now that we're >>>>> - * done processing it. >>>>> - */ >>>>> - if (cameraStream->type() == CameraStream::Type::Internal) >>>>> - cameraStream->putBuffer(src); >>>>> + return; > I read the patches 07-09/10. > > The new code looks to assume the number of buffers that require post > processing is only one. > I consider so from > 1.) return added here > 2.) no protection to the Status > 3.) no check to wait for the number of pot-processing in > streamProcessingComplete(), so that a capture request complete is sent > multiple times. > > I also think descriptors_ pop logic is wrong. > I think we ideally deny a request given in requestComplete if it is > not a top of descriptors_. > The top of descriptors is kept to the currently processing capture request. > I think we should have two queues here like following. > descriptors_ - for request processed by camera > descriptors_on_processing - for request processed by post processors > Then we pop descriptors_ in completeRequest and descriptors_on_processing. > > Even if a capture request doesn't require post processing, we should > add it to descriptors_on_processing to send it after processing for > all the preceding request is complete > > These may be resolved in the next patch. But this happens at least at > this patch if I understand correctly. The v3 *now* is a mere reference for future development on this front. We have split up design of HAL into 3 parts and have been internally discussing to even clean up more the implementation that we have today. It's still work-in-progress but I will posting the relevant modules of the design that will eventually lead up to setting up infrastructure required for async post-processors. More or less broadly: - We expect to have only queue only - to track each incoming request, irrespective of whether it requires post-processing or not - status variables will be set on Camera3RequestDescriptors-wide and then Post-Processing-wide separately - Async Post-processing somehow needs to be "cancellable" on demand - to satisfy flush use cases - Single point of sending back capture results ONLY a.k.a single process_capture_result These are the hard requirements we are tracking to clean up the implementation we have currently. It's substantial work plus keeping in mind not to regress CTS (which has happened recently, even hampering development on this front). I may / may not address reviews on this particular v3, since I will be more inclined to develop in line to our design goals, while treating this series and it's reviews just as a point of reference at this point. Thanks! > > -Hiro >>>>> } >>>>> >>>>> - captureResult.result = descriptor->resultMetadata_->get(); >>>>> - callbacks_->process_capture_result(callbacks_, &captureResult); >>>>> - >>>>> - descriptors_.pop_front(); >>>>> + /* >>>>> + * Mark the status on the descriptor as success as no processing >>>>> + * is neeeded. >>>>> + */ >>>>> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success; >>>>> + sendCaptureResults(); >>>>> } >>>>> >>>>> >>>>> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, >>>>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, >>>>> Camera3RequestDescriptor *request) >>>>> { >>>>> if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) { >>>>> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer >>>>> CAMERA3_MSG_ERROR_BUFFER); >>>>> } >>>>> } >>>>> + >>>>> + /* >>>>> + * Return the FrameBuffer to the CameraStream now that we're >>>>> + * done processing it. >>>>> + */ >>>>> + if (cameraStream->type() == CameraStream::Type::Internal) >>>>> + cameraStream->putBuffer(request->internalBuffer_); >>>>> + >>>>> + sendCaptureResults(); >>>>> +} >>>>> + >>>>> +void CameraDevice::sendCaptureResults() >>>>> +{ >>>>> + Camera3RequestDescriptor *d = descriptors_.front().get(); >> The variable is named descriptor everywhere else, let's keep a common >> naming scheme (I'm tempted to rename Camera3RequestDescriptor to >> Camera3Request, and thus also rename all variables accordingly, but >> that's for later, on top of this series). >> >>>> If I'm not mistaken, in case a request that doesn't need >>>> post-processing will complete while the post-processed one is still >>>> being worked on, this could lead to a completion order inversion >>>> >>>> processCaptureRequest(n) >>>> descriptors.push(n) >>>> >>>> processCaptureRequest(n+1) >>>> descriptors.push(n+1) >>>> >>>> equestComplete(n) >>>> postProcess(n) -----------> process(n) >>>> | >>>> .... ... >>>> | >>>> requestComplete(n+1) | >>>> sendCaptureResult(n+1) | >>>> d = descriptors.front() | >>>> process_capture_res(n) | >>>> | >>>> sendCaptureResult(n) <---------------- >>>> d = descritpros.front() >>>> process_capture_res(n + 1) >>>> >>> Nice flow diagram btw, I need to get better at that ;-) >>> >>> So, first of all sendCaptureResults() doesn't take an argument, but I >>> get your point that you are trying to match for which 'n' request, >>> sendCapture result is called... >>> >>> sendCaptureResults() works on the queue's head. It doesn't care from >>> where it's called; it can be called from requestComplete()[same thread] >>> or streamProcessingComplete()[different thread]. All it does or suppose >>> to do, is basically looks at the queue's head and see if the HEAD >>> descriptor says - "Send me back to the framework". >>> - If yes, it will send the results via process_capture_result of the >>> HEAD and drop the HEAD from the queue >>> - If no, simply return; >>> >>> That's it. Since, it looks at the head, it will only send results in >>> order (since it's a queue) and drop the HEAD. >>> >>> However, the discussion has un-covered another point that >>> sendCaptureResults() can lag behind w.r.t libcamera:Requests being >>> completed, since there is no loop around to process other descriptors in >>> the queue which are ready to send (these descriptors can potentially be >>> the ones not requiring any post-processing). As discussed in the call, >>> we both agree that we should probably iterate over the queue and send >>> the results back to the framework as soon as possible, for ready >>> descriptor. So, sendCaptureResults() will be wrapped in a loop in >>> subsequent version. >> I'd include the loop inside sendCaptureResults(). Something along the >> lines of >> >> MutexLocker lock(descriptorsMutex_); >> >> while (!descriptors_.empty() && >> descriptors_.front()->status_ != Camera3RequestDescriptor::Status::Pending) { >> std::unique_ptr<Camera3RequestDescriptor> descriptor = >> std::move(descriptors_.front()); >> descriptors_.pop(); >> >> lock.unlock(); >> >> /* Signal completion to the camera service. */ >> >> lock.lock(); >> } >> >> (see my review of 07/10 for the Pending status) >> >> Note that you'll need to handle both the Success and Error statuses >> here, as requests that complete with an error need to be completed in >> order too. >> >> One a side note, maybe adding the following helper to >> Camera3RequestDescriptor would be nice, to make the loop more readable: >> >> bool isPending() const { return status_ == Status::Pending; } >> >> You would then get >> >> while (!descriptors_.empty() && descriptors_.front()->isPending()) { >> ... >> } >> >>>>> + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || >>>>> + d->status_ == Camera3RequestDescriptor::ProcessStatus::None) >>>>> + return; >>>>> + >>>>> + MutexLocker lock(descriptorsMutex_); >>>>> + d->captureResult_.result = d->resultMetadata_->get(); >>>>> + callbacks_->process_capture_result(callbacks_, &(d->captureResult_)); >>>>> + descriptors_.pop_front(); >>>>> } >>>>> >>>>> std::string CameraDevice::logPrefix() const >>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >>>>> index 60c134dc..0bd570a1 100644 >>>>> --- a/src/android/camera_device.h >>>>> +++ b/src/android/camera_device.h >>>>> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor { >>>>> std::unique_ptr<CaptureRequest> request_; >>>>> std::unique_ptr<CameraMetadata> resultMetadata_; >>>>> >>>>> + camera3_capture_result_t captureResult_ = {}; >>>>> + libcamera::FrameBuffer *internalBuffer_; >>>>> + >>>>> ProcessStatus status_; >>>>> }; >>>>> >>>>> @@ -118,6 +121,7 @@ private: >>>>> int processControls(Camera3RequestDescriptor *descriptor); >>>>> std::unique_ptr<CameraMetadata> getResultMetadata( >>>>> const Camera3RequestDescriptor *descriptor) const; >>>>> + void sendCaptureResults(); >>>>> >>>>> unsigned int id_; >>>>> camera3_device_t camera3Device_; >> -- >> Regards, >> >> Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 4658e881..16ecdfc5 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1078,7 +1078,7 @@ 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 = {}; + 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_) { @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request) continue; } + if (cameraStream->type() == CameraStream::Type::Internal) + descriptor->internalBuffer_ = src; + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; cameraStream->process(src, *buffer.buffer, descriptor); - - /* - * Return the FrameBuffer to the CameraStream now that we're - * done processing it. - */ - if (cameraStream->type() == CameraStream::Type::Internal) - cameraStream->putBuffer(src); + return; } - captureResult.result = descriptor->resultMetadata_->get(); - callbacks_->process_capture_result(callbacks_, &captureResult); - - descriptors_.pop_front(); + /* + * Mark the status on the descriptor as success as no processing + * is neeeded. + */ + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success; + sendCaptureResults(); } -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream, Camera3RequestDescriptor *request) { if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) { @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer CAMERA3_MSG_ERROR_BUFFER); } } + + /* + * Return the FrameBuffer to the CameraStream now that we're + * done processing it. + */ + if (cameraStream->type() == CameraStream::Type::Internal) + cameraStream->putBuffer(request->internalBuffer_); + + sendCaptureResults(); +} + +void CameraDevice::sendCaptureResults() +{ + Camera3RequestDescriptor *d = descriptors_.front().get(); + if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || + d->status_ == Camera3RequestDescriptor::ProcessStatus::None) + return; + + MutexLocker lock(descriptorsMutex_); + d->captureResult_.result = d->resultMetadata_->get(); + callbacks_->process_capture_result(callbacks_, &(d->captureResult_)); + descriptors_.pop_front(); } std::string CameraDevice::logPrefix() const diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 60c134dc..0bd570a1 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor { std::unique_ptr<CaptureRequest> request_; std::unique_ptr<CameraMetadata> resultMetadata_; + camera3_capture_result_t captureResult_ = {}; + libcamera::FrameBuffer *internalBuffer_; + ProcessStatus status_; }; @@ -118,6 +121,7 @@ private: int processControls(Camera3RequestDescriptor *descriptor); std::unique_ptr<CameraMetadata> getResultMetadata( const Camera3RequestDescriptor *descriptor) const; + void sendCaptureResults(); unsigned int id_; camera3_device_t camera3Device_;
A Camera3RequestDescriptors is constructed and queued to descriptors_ queue as soon as an (incoming) capture request is received on the libcamera HAL. The capture request is picked up by HAL, in order to run it to completion. At completion, CameraDevice::requestComplete() gets invoked and capture results are populated and ready to be sent back to the framework. All the data and framebuffers associated with the request are alive and encapsulated inside this Camera3RequestDescriptor descriptor. By inspecting the ProcessStatus on the descriptor, we can now send capture results via the process_capture_result() callback. Hence, introduce a new private member function sendCaptureResults() which will be responsible to send capture results back to the framework by inspecting the descriptor on the queue. In subsequent commit, when the post processsor shall run async, sendCaptureResults() can be called from the post-processor's thread for e.g. streamProcessComplete() hence, introduce the mutex lock to avoid the races. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/camera_device.cpp | 47 +++++++++++++++++++++++++---------- src/android/camera_device.h | 4 +++ 2 files changed, 38 insertions(+), 13 deletions(-)