| Message ID | 20210920173752.1346190-8-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:49PM +0530, Umang Jain wrote: > We should be able to know if post-processing has been completed > successfully or encountered some errors. This commit introduces a > Signal<> that will notify that the post-processing has been completed. > If the post processing was successful, status on the request descriptor > will be set to Camera3RequestDescriptor::ProcessStatus::Success. > The signal will be required when the post-processor is meant to > run asynchronously (in subsequent commits) and capture results need > to be sent back to the framework from the signal's slot instead. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 18 ++++++++++++++++++ > src/android/camera_device.h | 11 +++++++++++ > src/android/camera_stream.cpp | 15 ++++++++++++++- > src/android/camera_stream.h | 2 ++ > src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++- > src/android/post_processor.h | 4 ++++ > src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++-- > 7 files changed, 72 insertions(+), 4 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index fa462368..e2de4012 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > */ > request_ = std::make_unique<CaptureRequest>(camera, > reinterpret_cast<uint64_t>(this)); > + > + /* > + * Denotes the post-processing status required by any stream. It is set > + * to ProcessStatus::Success if processing is successful. > + */ > + status_ = ProcessStatus::None; Looking at how the status is used in CameraDevice, I would rather make it a paramter of CameraDevice::streamProcessingComplete() and in the following patch of sendCaptureResults(). This way you could avoid there if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || d->status_ == Camera3RequestDescriptor::ProcessStatus::None) return; > } > > /* > @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request) > continue; > } > > + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; > + > int ret = cameraStream->process(src, *buffer.buffer, descriptor); > > /* > @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request) > descriptors_.pop_front(); > } > > + > +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, > + [[maybe_unused]] Camera3RequestDescriptor *request) > +{ > + /* > + * \todo Stream processing is completed hence, check for errors and > + * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR. > + */ > +} > + > std::string CameraDevice::logPrefix() const > { > return "'" + camera_->id() + "'"; > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index b2871e52..60c134dc 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -36,6 +36,13 @@ > struct CameraConfigData; > > struct Camera3RequestDescriptor { > + enum class ProcessStatus { > + None, > + Processing, > + Success, > + Error > + }; > + > Camera3RequestDescriptor() = default; > ~Camera3RequestDescriptor() = default; > Camera3RequestDescriptor(libcamera::Camera *camera, > @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor { > CameraMetadata settings_; > std::unique_ptr<CaptureRequest> request_; > std::unique_ptr<CameraMetadata> resultMetadata_; > + > + ProcessStatus status_; > }; > > class CameraDevice : protected libcamera::Loggable > @@ -79,6 +88,8 @@ public: > int configureStreams(camera3_stream_configuration_t *stream_list); > int processCaptureRequest(camera3_capture_request_t *request); > void requestComplete(libcamera::Request *request); > + void streamProcessingComplete(CameraStream *cameraStream, > + Camera3RequestDescriptor *request); > > protected: > std::string logPrefix() const override; > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index d494f5d5..70471959 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -81,6 +81,9 @@ int CameraStream::configure() > int ret = postProcessor_->configure(configuration(), output); > if (ret) > return ret; > + > + postProcessor_->processComplete.connect( > + this, &CameraStream::handleProcessComplete); > } > > if (allocator_) { > @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source, > buffer_handle_t camera3Dest, > Camera3RequestDescriptor *request) > { > - if (!postProcessor_) > + if (!postProcessor_) { > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + handleProcessComplete(request); > return 0; > + } That was like this before, but I feel like this is a development mistaken and should be better caught with Fatal same for the below !encoder and the YUV post-processor > > /* > * \todo Buffer mapping and processing should be moved to a > @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source, > PROT_READ | PROT_WRITE); > if (!dest.isValid()) { > LOG(HAL, Error) << "Failed to map android blob buffer"; > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + handleProcessComplete(request); > return -EINVAL; > } > > return postProcessor_->process(source, &dest, request); > } > > +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request) > +{ > + cameraDevice_->streamProcessingComplete(this, request); > +} Can't CameraStream connect CameraDevice::streamProcessingComplete with the post-processor's processComplete signal ? > + > FrameBuffer *CameraStream::getBuffer() > { > if (!allocator_) > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index 68789700..f8ad6245 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -127,6 +127,8 @@ public: > void putBuffer(libcamera::FrameBuffer *buffer); > > private: > + void handleProcessComplete(Camera3RequestDescriptor *request); > + > CameraDevice *const cameraDevice_; > const libcamera::CameraConfiguration *config_; > const Type type_; > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 31f68330..87252acd 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > CameraBuffer *destination, > Camera3RequestDescriptor *request) > { > - if (!encoder_) > + if (!encoder_) { > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return 0; > + } > > ASSERT(destination->numPlanes() == 1); > > @@ -197,6 +200,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > exif.data(), quality); > if (jpeg_size < 0) { > LOG(JPEG, Error) << "Failed to encode stream image"; > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return jpeg_size; > } > > @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > /* Update the JPEG result Metadata. */ > resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > + processComplete.emit(request); > + > return 0; > } > diff --git a/src/android/post_processor.h b/src/android/post_processor.h > index bdd6afe7..f639b237 100644 > --- a/src/android/post_processor.h > +++ b/src/android/post_processor.h > @@ -7,6 +7,8 @@ > #ifndef __ANDROID_POST_PROCESSOR_H__ > #define __ANDROID_POST_PROCESSOR_H__ > > +#include <libcamera/base/signal.h> > + > #include <libcamera/framebuffer.h> > #include <libcamera/stream.h> > > @@ -26,6 +28,8 @@ public: > virtual int process(const libcamera::FrameBuffer *source, > CameraBuffer *destination, > Camera3RequestDescriptor *request) = 0; > + > + libcamera::Signal<Camera3RequestDescriptor *> processComplete; > }; > > #endif /* __ANDROID_POST_PROCESSOR_H__ */ > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > index 5e18caee..b144649a 100644 > --- a/src/android/yuv/post_processor_yuv.cpp > +++ b/src/android/yuv/post_processor_yuv.cpp > @@ -18,6 +18,8 @@ > #include "libcamera/internal/formats.h" > #include "libcamera/internal/mapped_framebuffer.h" > > +#include "../camera_device.h" > + > using namespace libcamera; > > LOG_DEFINE_CATEGORY(YUV) > @@ -51,14 +53,19 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, > > int PostProcessorYuv::process(const FrameBuffer *source, > CameraBuffer *destination, > - [[maybe_unused]] Camera3RequestDescriptor *request) > + Camera3RequestDescriptor *request) > { > - if (!isValidBuffers(source, *destination)) > + if (!isValidBuffers(source, *destination)) { > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); If my first suggestion will be considered, the post processor will send the status in the signal, and you can reduce the number of status to Success or Error maybe ? > return -EINVAL; > + } > > const MappedFrameBuffer sourceMapped(source, MappedFrameBuffer::MapFlag::Read); > if (!sourceMapped.isValid()) { > LOG(YUV, Error) << "Failed to mmap camera frame buffer"; > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return -EINVAL; > } > > @@ -76,9 +83,14 @@ int PostProcessorYuv::process(const FrameBuffer *source, > libyuv::FilterMode::kFilterBilinear); > if (ret) { > LOG(YUV, Error) << "Failed NV12 scaling: " << ret; > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return -EINVAL; > } > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > + processComplete.emit(request); > + > return 0; > } > > -- > 2.31.1 >
On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote: > We should be able to know if post-processing has been completed > successfully or encountered some errors. This commit introduces a > Signal<> that will notify that the post-processing has been completed. > If the post processing was successful, status on the request descriptor > will be set to Camera3RequestDescriptor::ProcessStatus::Success. > The signal will be required when the post-processor is meant to > run asynchronously (in subsequent commits) and capture results need > to be sent back to the framework from the signal's slot instead. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_device.cpp | 18 ++++++++++++++++++ > src/android/camera_device.h | 11 +++++++++++ > src/android/camera_stream.cpp | 15 ++++++++++++++- > src/android/camera_stream.h | 2 ++ > src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++- > src/android/post_processor.h | 4 ++++ > src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++-- > 7 files changed, 72 insertions(+), 4 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index fa462368..e2de4012 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > */ > request_ = std::make_unique<CaptureRequest>(camera, > reinterpret_cast<uint64_t>(this)); > + > + /* > + * Denotes the post-processing status required by any stream. It is set > + * to ProcessStatus::Success if processing is successful. > + */ > + status_ = ProcessStatus::None; > } > > /* > @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request) > continue; > } > > + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; > + > int ret = cameraStream->process(src, *buffer.buffer, descriptor); > > /* > @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request) > descriptors_.pop_front(); > } > > + Also double empty line weird that checkstyle does not complain > +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, > + [[maybe_unused]] Camera3RequestDescriptor *request) > +{ > + /* > + * \todo Stream processing is completed hence, check for errors and > + * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR. > + */ > +} > + > std::string CameraDevice::logPrefix() const > { > return "'" + camera_->id() + "'"; > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index b2871e52..60c134dc 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -36,6 +36,13 @@ > struct CameraConfigData; > > struct Camera3RequestDescriptor { > + enum class ProcessStatus { > + None, > + Processing, > + Success, > + Error > + }; > + > Camera3RequestDescriptor() = default; > ~Camera3RequestDescriptor() = default; > Camera3RequestDescriptor(libcamera::Camera *camera, > @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor { > CameraMetadata settings_; > std::unique_ptr<CaptureRequest> request_; > std::unique_ptr<CameraMetadata> resultMetadata_; > + > + ProcessStatus status_; > }; > > class CameraDevice : protected libcamera::Loggable > @@ -79,6 +88,8 @@ public: > int configureStreams(camera3_stream_configuration_t *stream_list); > int processCaptureRequest(camera3_capture_request_t *request); > void requestComplete(libcamera::Request *request); > + void streamProcessingComplete(CameraStream *cameraStream, > + Camera3RequestDescriptor *request); > > protected: > std::string logPrefix() const override; > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index d494f5d5..70471959 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -81,6 +81,9 @@ int CameraStream::configure() > int ret = postProcessor_->configure(configuration(), output); > if (ret) > return ret; > + > + postProcessor_->processComplete.connect( > + this, &CameraStream::handleProcessComplete); > } > > if (allocator_) { > @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source, > buffer_handle_t camera3Dest, > Camera3RequestDescriptor *request) > { > - if (!postProcessor_) > + if (!postProcessor_) { > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + handleProcessComplete(request); > return 0; > + } > > /* > * \todo Buffer mapping and processing should be moved to a > @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source, > PROT_READ | PROT_WRITE); > if (!dest.isValid()) { > LOG(HAL, Error) << "Failed to map android blob buffer"; > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + handleProcessComplete(request); > return -EINVAL; > } > > return postProcessor_->process(source, &dest, request); > } > > +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request) > +{ > + cameraDevice_->streamProcessingComplete(this, request); > +} > + > FrameBuffer *CameraStream::getBuffer() > { > if (!allocator_) > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index 68789700..f8ad6245 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -127,6 +127,8 @@ public: > void putBuffer(libcamera::FrameBuffer *buffer); > > private: > + void handleProcessComplete(Camera3RequestDescriptor *request); > + > CameraDevice *const cameraDevice_; > const libcamera::CameraConfiguration *config_; > const Type type_; > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 31f68330..87252acd 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > CameraBuffer *destination, > Camera3RequestDescriptor *request) > { > - if (!encoder_) > + if (!encoder_) { > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return 0; > + } > > ASSERT(destination->numPlanes() == 1); > > @@ -197,6 +200,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > exif.data(), quality); > if (jpeg_size < 0) { > LOG(JPEG, Error) << "Failed to encode stream image"; > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return jpeg_size; > } > > @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > /* Update the JPEG result Metadata. */ > resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > + processComplete.emit(request); > + > return 0; > } > diff --git a/src/android/post_processor.h b/src/android/post_processor.h > index bdd6afe7..f639b237 100644 > --- a/src/android/post_processor.h > +++ b/src/android/post_processor.h > @@ -7,6 +7,8 @@ > #ifndef __ANDROID_POST_PROCESSOR_H__ > #define __ANDROID_POST_PROCESSOR_H__ > > +#include <libcamera/base/signal.h> > + > #include <libcamera/framebuffer.h> > #include <libcamera/stream.h> > > @@ -26,6 +28,8 @@ public: > virtual int process(const libcamera::FrameBuffer *source, > CameraBuffer *destination, > Camera3RequestDescriptor *request) = 0; > + > + libcamera::Signal<Camera3RequestDescriptor *> processComplete; > }; > > #endif /* __ANDROID_POST_PROCESSOR_H__ */ > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > index 5e18caee..b144649a 100644 > --- a/src/android/yuv/post_processor_yuv.cpp > +++ b/src/android/yuv/post_processor_yuv.cpp > @@ -18,6 +18,8 @@ > #include "libcamera/internal/formats.h" > #include "libcamera/internal/mapped_framebuffer.h" > > +#include "../camera_device.h" > + > using namespace libcamera; > > LOG_DEFINE_CATEGORY(YUV) > @@ -51,14 +53,19 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, > > int PostProcessorYuv::process(const FrameBuffer *source, > CameraBuffer *destination, > - [[maybe_unused]] Camera3RequestDescriptor *request) > + Camera3RequestDescriptor *request) > { > - if (!isValidBuffers(source, *destination)) > + if (!isValidBuffers(source, *destination)) { > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return -EINVAL; > + } > > const MappedFrameBuffer sourceMapped(source, MappedFrameBuffer::MapFlag::Read); > if (!sourceMapped.isValid()) { > LOG(YUV, Error) << "Failed to mmap camera frame buffer"; > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return -EINVAL; > } > > @@ -76,9 +83,14 @@ int PostProcessorYuv::process(const FrameBuffer *source, > libyuv::FilterMode::kFilterBilinear); > if (ret) { > LOG(YUV, Error) << "Failed NV12 scaling: " << ret; > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > + processComplete.emit(request); > return -EINVAL; > } > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > + processComplete.emit(request); > + > return 0; > } > > -- > 2.31.1 >
Hello, On Tue, Sep 21, 2021 at 01:29:28PM +0200, Jacopo Mondi wrote: > On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote: > > We should be able to know if post-processing has been completed > > successfully or encountered some errors. This commit introduces a > > Signal<> that will notify that the post-processing has been completed. > > If the post processing was successful, status on the request descriptor > > will be set to Camera3RequestDescriptor::ProcessStatus::Success. > > The signal will be required when the post-processor is meant to > > run asynchronously (in subsequent commits) and capture results need > > to be sent back to the framework from the signal's slot instead. > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 18 ++++++++++++++++++ > > src/android/camera_device.h | 11 +++++++++++ > > src/android/camera_stream.cpp | 15 ++++++++++++++- > > src/android/camera_stream.h | 2 ++ > > src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++- > > src/android/post_processor.h | 4 ++++ > > src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++-- > > 7 files changed, 72 insertions(+), 4 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index fa462368..e2de4012 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > > */ > > request_ = std::make_unique<CaptureRequest>(camera, > > reinterpret_cast<uint64_t>(this)); > > + > > + /* > > + * Denotes the post-processing status required by any stream. It is set > > + * to ProcessStatus::Success if processing is successful. > > + */ > > + status_ = ProcessStatus::None; > > Looking at how the status is used in CameraDevice, I would rather make > it a paramter of CameraDevice::streamProcessingComplete() and in the > following patch of sendCaptureResults(). > > This way you could avoid there > if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || > d->status_ == Camera3RequestDescriptor::ProcessStatus::None) > return; I'm in two minds about this. For the PostProcessor::processComplete() signal, I agree, we could pass a status back as a signal parameter. It would have the advantage of not having to set the request status in quite a few locations in the post-processors. In that case, the status should be a enum class Status { Success, Error, }; in the PostProcessor class, as we don't need more than that. However, once we reach CameraStream::handleProcessComplete(), I think it would make sense to store the status in Camera3RequestDescriptor, to keep all request-related data together. This will be needed when iterating over all descriptors in the queue to find out which ones are complete. We would need a different Camera3RequestDescriptor::Status there, and I think it should indicate the status of the request, not the status of the post-processing. We would thus have enum class Status { Pending, Success, Error, }; in Camera3RequestDescriptor. > > } > > > > /* > > @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request) > > continue; > > } > > > > + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; > > + > > int ret = cameraStream->process(src, *buffer.buffer, descriptor); > > > > /* > > @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request) > > descriptors_.pop_front(); > > } > > > > + > > +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, > > + [[maybe_unused]] Camera3RequestDescriptor *request) > > +{ > > + /* > > + * \todo Stream processing is completed hence, check for errors and > > + * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR. > > + */ > > +} > > + > > std::string CameraDevice::logPrefix() const > > { > > return "'" + camera_->id() + "'"; > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index b2871e52..60c134dc 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -36,6 +36,13 @@ > > struct CameraConfigData; > > > > struct Camera3RequestDescriptor { > > + enum class ProcessStatus { > > + None, > > + Processing, > > + Success, > > + Error > > + }; > > + > > Camera3RequestDescriptor() = default; > > ~Camera3RequestDescriptor() = default; > > Camera3RequestDescriptor(libcamera::Camera *camera, > > @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor { > > CameraMetadata settings_; > > std::unique_ptr<CaptureRequest> request_; > > std::unique_ptr<CameraMetadata> resultMetadata_; > > + > > + ProcessStatus status_; > > }; > > > > class CameraDevice : protected libcamera::Loggable > > @@ -79,6 +88,8 @@ public: > > int configureStreams(camera3_stream_configuration_t *stream_list); > > int processCaptureRequest(camera3_capture_request_t *request); > > void requestComplete(libcamera::Request *request); > > + void streamProcessingComplete(CameraStream *cameraStream, > > + Camera3RequestDescriptor *request); > > > > protected: > > std::string logPrefix() const override; > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > > index d494f5d5..70471959 100644 > > --- a/src/android/camera_stream.cpp > > +++ b/src/android/camera_stream.cpp > > @@ -81,6 +81,9 @@ int CameraStream::configure() > > int ret = postProcessor_->configure(configuration(), output); > > if (ret) > > return ret; > > + > > + postProcessor_->processComplete.connect( > > + this, &CameraStream::handleProcessComplete); > > } > > > > if (allocator_) { > > @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source, > > buffer_handle_t camera3Dest, > > Camera3RequestDescriptor *request) > > { > > - if (!postProcessor_) > > + if (!postProcessor_) { > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > > + handleProcessComplete(request); > > return 0; > > + } > > That was like this before, but I feel like this is a development > mistaken and should be better caught with Fatal > > same for the below !encoder and the YUV post-processor Or ASSERT(postProcessor_); ? > > > > /* > > * \todo Buffer mapping and processing should be moved to a > > @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source, > > PROT_READ | PROT_WRITE); > > if (!dest.isValid()) { > > LOG(HAL, Error) << "Failed to map android blob buffer"; > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > > + handleProcessComplete(request); > > return -EINVAL; > > } > > > > return postProcessor_->process(source, &dest, request); > > } > > > > +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request) > > +{ > > + cameraDevice_->streamProcessingComplete(this, request); > > +} > > Can't CameraStream connect CameraDevice::streamProcessingComplete with > the post-processor's processComplete signal ? We can do so with a lambda function, now supported by the signal class: postProcessor_->processComplete.connect( [&](Camera3RequestDescriptor *request) { cameraDevice_->streamProcessingComplete(this, request); } ); > > + > > FrameBuffer *CameraStream::getBuffer() > > { > > if (!allocator_) > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > > index 68789700..f8ad6245 100644 > > --- a/src/android/camera_stream.h > > +++ b/src/android/camera_stream.h > > @@ -127,6 +127,8 @@ public: > > void putBuffer(libcamera::FrameBuffer *buffer); > > > > private: > > + void handleProcessComplete(Camera3RequestDescriptor *request); > > + > > CameraDevice *const cameraDevice_; > > const libcamera::CameraConfiguration *config_; > > const Type type_; > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > > index 31f68330..87252acd 100644 > > --- a/src/android/jpeg/post_processor_jpeg.cpp > > +++ b/src/android/jpeg/post_processor_jpeg.cpp > > @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > > CameraBuffer *destination, > > Camera3RequestDescriptor *request) > > { > > - if (!encoder_) > > + if (!encoder_) { > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > > + processComplete.emit(request); > > return 0; > > + } > > > > ASSERT(destination->numPlanes() == 1); > > > > @@ -197,6 +200,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > > exif.data(), quality); > > if (jpeg_size < 0) { > > LOG(JPEG, Error) << "Failed to encode stream image"; > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > > + processComplete.emit(request); > > return jpeg_size; > > } > > > > @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > > /* Update the JPEG result Metadata. */ > > resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); > > > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > > + processComplete.emit(request); > > + > > return 0; > > } > > diff --git a/src/android/post_processor.h b/src/android/post_processor.h > > index bdd6afe7..f639b237 100644 > > --- a/src/android/post_processor.h > > +++ b/src/android/post_processor.h > > @@ -7,6 +7,8 @@ > > #ifndef __ANDROID_POST_PROCESSOR_H__ > > #define __ANDROID_POST_PROCESSOR_H__ > > > > +#include <libcamera/base/signal.h> > > + > > #include <libcamera/framebuffer.h> > > #include <libcamera/stream.h> > > > > @@ -26,6 +28,8 @@ public: > > virtual int process(const libcamera::FrameBuffer *source, > > CameraBuffer *destination, > > Camera3RequestDescriptor *request) = 0; > > + > > + libcamera::Signal<Camera3RequestDescriptor *> processComplete; > > }; > > > > #endif /* __ANDROID_POST_PROCESSOR_H__ */ > > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > > index 5e18caee..b144649a 100644 > > --- a/src/android/yuv/post_processor_yuv.cpp > > +++ b/src/android/yuv/post_processor_yuv.cpp > > @@ -18,6 +18,8 @@ > > #include "libcamera/internal/formats.h" > > #include "libcamera/internal/mapped_framebuffer.h" > > > > +#include "../camera_device.h" > > + > > using namespace libcamera; > > > > LOG_DEFINE_CATEGORY(YUV) > > @@ -51,14 +53,19 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, > > > > int PostProcessorYuv::process(const FrameBuffer *source, > > CameraBuffer *destination, > > - [[maybe_unused]] Camera3RequestDescriptor *request) > > + Camera3RequestDescriptor *request) > > { > > - if (!isValidBuffers(source, *destination)) > > + if (!isValidBuffers(source, *destination)) { > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > > + processComplete.emit(request); > > If my first suggestion will be considered, the post processor will > send the status in the signal, and you can reduce the number of status > to Success or Error maybe ? > > > return -EINVAL; > > + } > > > > const MappedFrameBuffer sourceMapped(source, MappedFrameBuffer::MapFlag::Read); > > if (!sourceMapped.isValid()) { > > LOG(YUV, Error) << "Failed to mmap camera frame buffer"; > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > > + processComplete.emit(request); > > return -EINVAL; > > } > > > > @@ -76,9 +83,14 @@ int PostProcessorYuv::process(const FrameBuffer *source, > > libyuv::FilterMode::kFilterBilinear); > > if (ret) { > > LOG(YUV, Error) << "Failed NV12 scaling: " << ret; > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > > + processComplete.emit(request); > > return -EINVAL; > > } > > > > + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > > + processComplete.emit(request); > > + > > return 0; > > } > >
Hi Laurent, On 9/22/21 4:56 AM, Laurent Pinchart wrote: > Hello, > > On Tue, Sep 21, 2021 at 01:29:28PM +0200, Jacopo Mondi wrote: >> On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote: >>> We should be able to know if post-processing has been completed >>> successfully or encountered some errors. This commit introduces a >>> Signal<> that will notify that the post-processing has been completed. >>> If the post processing was successful, status on the request descriptor >>> will be set to Camera3RequestDescriptor::ProcessStatus::Success. >>> The signal will be required when the post-processor is meant to >>> run asynchronously (in subsequent commits) and capture results need >>> to be sent back to the framework from the signal's slot instead. >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> src/android/camera_device.cpp | 18 ++++++++++++++++++ >>> src/android/camera_device.h | 11 +++++++++++ >>> src/android/camera_stream.cpp | 15 ++++++++++++++- >>> src/android/camera_stream.h | 2 ++ >>> src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++- >>> src/android/post_processor.h | 4 ++++ >>> src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++-- >>> 7 files changed, 72 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index fa462368..e2de4012 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( >>> */ >>> request_ = std::make_unique<CaptureRequest>(camera, >>> reinterpret_cast<uint64_t>(this)); >>> + >>> + /* >>> + * Denotes the post-processing status required by any stream. It is set >>> + * to ProcessStatus::Success if processing is successful. >>> + */ >>> + status_ = ProcessStatus::None; >> Looking at how the status is used in CameraDevice, I would rather make >> it a paramter of CameraDevice::streamProcessingComplete() and in the >> following patch of sendCaptureResults(). >> >> This way you could avoid there >> if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || >> d->status_ == Camera3RequestDescriptor::ProcessStatus::None) >> return; > I'm in two minds about this. For the PostProcessor::processComplete() > signal, I agree, we could pass a status back as a signal parameter. It > would have the advantage of not having to set the request status in > quite a few locations in the post-processors. In that case, the status > should be a > > enum class Status { > Success, > Error, > }; > > in the PostProcessor class, as we don't need more than that. > > However, once we reach CameraStream::handleProcessComplete(), I think it > would make sense to store the status in Camera3RequestDescriptor, to > keep all request-related data together. This will be needed when > iterating over all descriptors in the queue to find out which ones are > complete. We would need a different Camera3RequestDescriptor::Status > there, and I think it should indicate the status of the request, not the > status of the post-processing. We would thus have > > enum class Status { > Pending, > Success, > Error, > }; > > in Camera3RequestDescriptor. Ok so I need to split status variables out into two separate enums? - One would target PostProcessor Status and sit in CameraStream - Other would target the descriptor's status and sit in Camera3RequestDesriptor I think we agreed upon a centralized location of status variables hence, it made sense to keep them in the descriptor itself I find the 2 split approach decent as well, so it should be fine. > >>> } >>> >>> /* >>> @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request) >>> continue; >>> } >>> >>> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; >>> + >>> int ret = cameraStream->process(src, *buffer.buffer, descriptor); >>> >>> /* >>> @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request) >>> descriptors_.pop_front(); >>> } >>> >>> + >>> +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, >>> + [[maybe_unused]] Camera3RequestDescriptor *request) >>> +{ >>> + /* >>> + * \todo Stream processing is completed hence, check for errors and >>> + * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR. >>> + */ >>> +} >>> + >>> std::string CameraDevice::logPrefix() const >>> { >>> return "'" + camera_->id() + "'"; >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >>> index b2871e52..60c134dc 100644 >>> --- a/src/android/camera_device.h >>> +++ b/src/android/camera_device.h >>> @@ -36,6 +36,13 @@ >>> struct CameraConfigData; >>> >>> struct Camera3RequestDescriptor { >>> + enum class ProcessStatus { >>> + None, >>> + Processing, >>> + Success, >>> + Error >>> + }; >>> + >>> Camera3RequestDescriptor() = default; >>> ~Camera3RequestDescriptor() = default; >>> Camera3RequestDescriptor(libcamera::Camera *camera, >>> @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor { >>> CameraMetadata settings_; >>> std::unique_ptr<CaptureRequest> request_; >>> std::unique_ptr<CameraMetadata> resultMetadata_; >>> + >>> + ProcessStatus status_; >>> }; >>> >>> class CameraDevice : protected libcamera::Loggable >>> @@ -79,6 +88,8 @@ public: >>> int configureStreams(camera3_stream_configuration_t *stream_list); >>> int processCaptureRequest(camera3_capture_request_t *request); >>> void requestComplete(libcamera::Request *request); >>> + void streamProcessingComplete(CameraStream *cameraStream, >>> + Camera3RequestDescriptor *request); >>> >>> protected: >>> std::string logPrefix() const override; >>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp >>> index d494f5d5..70471959 100644 >>> --- a/src/android/camera_stream.cpp >>> +++ b/src/android/camera_stream.cpp >>> @@ -81,6 +81,9 @@ int CameraStream::configure() >>> int ret = postProcessor_->configure(configuration(), output); >>> if (ret) >>> return ret; >>> + >>> + postProcessor_->processComplete.connect( >>> + this, &CameraStream::handleProcessComplete); >>> } >>> >>> if (allocator_) { >>> @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source, >>> buffer_handle_t camera3Dest, >>> Camera3RequestDescriptor *request) >>> { >>> - if (!postProcessor_) >>> + if (!postProcessor_) { >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; >>> + handleProcessComplete(request); >>> return 0; >>> + } >> That was like this before, but I feel like this is a development >> mistaken and should be better caught with Fatal >> >> same for the below !encoder and the YUV post-processor > Or > > ASSERT(postProcessor_); > > ? > >>> /* >>> * \todo Buffer mapping and processing should be moved to a >>> @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source, >>> PROT_READ | PROT_WRITE); >>> if (!dest.isValid()) { >>> LOG(HAL, Error) << "Failed to map android blob buffer"; >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; >>> + handleProcessComplete(request); >>> return -EINVAL; >>> } >>> >>> return postProcessor_->process(source, &dest, request); >>> } >>> >>> +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request) >>> +{ >>> + cameraDevice_->streamProcessingComplete(this, request); >>> +} >> Can't CameraStream connect CameraDevice::streamProcessingComplete with >> the post-processor's processComplete signal ? > We can do so with a lambda function, now supported by the signal class: > > postProcessor_->processComplete.connect( > [&](Camera3RequestDescriptor *request) { > cameraDevice_->streamProcessingComplete(this, request); > } > ); > >>> + >>> FrameBuffer *CameraStream::getBuffer() >>> { >>> if (!allocator_) >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h >>> index 68789700..f8ad6245 100644 >>> --- a/src/android/camera_stream.h >>> +++ b/src/android/camera_stream.h >>> @@ -127,6 +127,8 @@ public: >>> void putBuffer(libcamera::FrameBuffer *buffer); >>> >>> private: >>> + void handleProcessComplete(Camera3RequestDescriptor *request); >>> + >>> CameraDevice *const cameraDevice_; >>> const libcamera::CameraConfiguration *config_; >>> const Type type_; >>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp >>> index 31f68330..87252acd 100644 >>> --- a/src/android/jpeg/post_processor_jpeg.cpp >>> +++ b/src/android/jpeg/post_processor_jpeg.cpp >>> @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source, >>> CameraBuffer *destination, >>> Camera3RequestDescriptor *request) >>> { >>> - if (!encoder_) >>> + if (!encoder_) { >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; >>> + processComplete.emit(request); >>> return 0; >>> + } >>> >>> ASSERT(destination->numPlanes() == 1); >>> >>> @@ -197,6 +200,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, >>> exif.data(), quality); >>> if (jpeg_size < 0) { >>> LOG(JPEG, Error) << "Failed to encode stream image"; >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; >>> + processComplete.emit(request); >>> return jpeg_size; >>> } >>> >>> @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, >>> /* Update the JPEG result Metadata. */ >>> resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); >>> >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; >>> + processComplete.emit(request); >>> + >>> return 0; >>> } >>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h >>> index bdd6afe7..f639b237 100644 >>> --- a/src/android/post_processor.h >>> +++ b/src/android/post_processor.h >>> @@ -7,6 +7,8 @@ >>> #ifndef __ANDROID_POST_PROCESSOR_H__ >>> #define __ANDROID_POST_PROCESSOR_H__ >>> >>> +#include <libcamera/base/signal.h> >>> + >>> #include <libcamera/framebuffer.h> >>> #include <libcamera/stream.h> >>> >>> @@ -26,6 +28,8 @@ public: >>> virtual int process(const libcamera::FrameBuffer *source, >>> CameraBuffer *destination, >>> Camera3RequestDescriptor *request) = 0; >>> + >>> + libcamera::Signal<Camera3RequestDescriptor *> processComplete; >>> }; >>> >>> #endif /* __ANDROID_POST_PROCESSOR_H__ */ >>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp >>> index 5e18caee..b144649a 100644 >>> --- a/src/android/yuv/post_processor_yuv.cpp >>> +++ b/src/android/yuv/post_processor_yuv.cpp >>> @@ -18,6 +18,8 @@ >>> #include "libcamera/internal/formats.h" >>> #include "libcamera/internal/mapped_framebuffer.h" >>> >>> +#include "../camera_device.h" >>> + >>> using namespace libcamera; >>> >>> LOG_DEFINE_CATEGORY(YUV) >>> @@ -51,14 +53,19 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, >>> >>> int PostProcessorYuv::process(const FrameBuffer *source, >>> CameraBuffer *destination, >>> - [[maybe_unused]] Camera3RequestDescriptor *request) >>> + Camera3RequestDescriptor *request) >>> { >>> - if (!isValidBuffers(source, *destination)) >>> + if (!isValidBuffers(source, *destination)) { >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; >>> + processComplete.emit(request); >> If my first suggestion will be considered, the post processor will >> send the status in the signal, and you can reduce the number of status >> to Success or Error maybe ? >> >>> return -EINVAL; >>> + } >>> >>> const MappedFrameBuffer sourceMapped(source, MappedFrameBuffer::MapFlag::Read); >>> if (!sourceMapped.isValid()) { >>> LOG(YUV, Error) << "Failed to mmap camera frame buffer"; >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; >>> + processComplete.emit(request); >>> return -EINVAL; >>> } >>> >>> @@ -76,9 +83,14 @@ int PostProcessorYuv::process(const FrameBuffer *source, >>> libyuv::FilterMode::kFilterBilinear); >>> if (ret) { >>> LOG(YUV, Error) << "Failed NV12 scaling: " << ret; >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; >>> + processComplete.emit(request); >>> return -EINVAL; >>> } >>> >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; >>> + processComplete.emit(request); >>> + >>> return 0; >>> } >>>
Hi Umang, On Wed, Sep 22, 2021 at 02:13:56PM +0530, Umang Jain wrote: > On 9/22/21 4:56 AM, Laurent Pinchart wrote: > > On Tue, Sep 21, 2021 at 01:29:28PM +0200, Jacopo Mondi wrote: > >> On Mon, Sep 20, 2021 at 11:07:49PM +0530, Umang Jain wrote: > >>> We should be able to know if post-processing has been completed > >>> successfully or encountered some errors. This commit introduces a > >>> Signal<> that will notify that the post-processing has been completed. > >>> If the post processing was successful, status on the request descriptor > >>> will be set to Camera3RequestDescriptor::ProcessStatus::Success. > >>> The signal will be required when the post-processor is meant to > >>> run asynchronously (in subsequent commits) and capture results need > >>> to be sent back to the framework from the signal's slot instead. > >>> > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>> --- > >>> src/android/camera_device.cpp | 18 ++++++++++++++++++ > >>> src/android/camera_device.h | 11 +++++++++++ > >>> src/android/camera_stream.cpp | 15 ++++++++++++++- > >>> src/android/camera_stream.h | 2 ++ > >>> src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++- > >>> src/android/post_processor.h | 4 ++++ > >>> src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++-- > >>> 7 files changed, 72 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>> index fa462368..e2de4012 100644 > >>> --- a/src/android/camera_device.cpp > >>> +++ b/src/android/camera_device.cpp > >>> @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > >>> */ > >>> request_ = std::make_unique<CaptureRequest>(camera, > >>> reinterpret_cast<uint64_t>(this)); > >>> + > >>> + /* > >>> + * Denotes the post-processing status required by any stream. It is set > >>> + * to ProcessStatus::Success if processing is successful. > >>> + */ > >>> + status_ = ProcessStatus::None; > >> Looking at how the status is used in CameraDevice, I would rather make > >> it a paramter of CameraDevice::streamProcessingComplete() and in the > >> following patch of sendCaptureResults(). > >> > >> This way you could avoid there > >> if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing || > >> d->status_ == Camera3RequestDescriptor::ProcessStatus::None) > >> return; > > I'm in two minds about this. For the PostProcessor::processComplete() > > signal, I agree, we could pass a status back as a signal parameter. It > > would have the advantage of not having to set the request status in > > quite a few locations in the post-processors. In that case, the status > > should be a > > > > enum class Status { > > Success, > > Error, > > }; > > > > in the PostProcessor class, as we don't need more than that. > > > > However, once we reach CameraStream::handleProcessComplete(), I think it > > would make sense to store the status in Camera3RequestDescriptor, to > > keep all request-related data together. This will be needed when > > iterating over all descriptors in the queue to find out which ones are > > complete. We would need a different Camera3RequestDescriptor::Status > > there, and I think it should indicate the status of the request, not the > > status of the post-processing. We would thus have > > > > enum class Status { > > Pending, > > Success, > > Error, > > }; > > > > in Camera3RequestDescriptor. > > Ok so I need to split status variables out into two separate enums? > > - One would target PostProcessor Status and sit in CameraStream The PostProcessor status enum should be in the PostProcessor class I think. It will only be used in the post-processing completion signal, it won't be stored in Camera3RequestDesriptor. On a side note, as this enum will only have two values, I think it would make sense to define a generic enum class Status { Success, Error } (or similar) in libcamera that could be reused everywhere when we need a boolean result. > - Other would target the descriptor's status and sit in > Camera3RequestDesriptor Correct. > I think we agreed upon a centralized location of status variables hence, > it made sense to keep them in the descriptor itself Yes, you'll need it to implement completion, by looping over the queue until you find a descriptor that has a Pending status. > I find the 2 split approach decent as well, so it should be fine. > > >>> } > >>> > >>> /* > >>> @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request) > >>> continue; > >>> } > >>> > >>> + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; > >>> + > >>> int ret = cameraStream->process(src, *buffer.buffer, descriptor); > >>> > >>> /* > >>> @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request) > >>> descriptors_.pop_front(); > >>> } > >>> > >>> + > >>> +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, > >>> + [[maybe_unused]] Camera3RequestDescriptor *request) > >>> +{ > >>> + /* > >>> + * \todo Stream processing is completed hence, check for errors and > >>> + * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR. > >>> + */ > >>> +} > >>> + > >>> std::string CameraDevice::logPrefix() const > >>> { > >>> return "'" + camera_->id() + "'"; > >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > >>> index b2871e52..60c134dc 100644 > >>> --- a/src/android/camera_device.h > >>> +++ b/src/android/camera_device.h > >>> @@ -36,6 +36,13 @@ > >>> struct CameraConfigData; > >>> > >>> struct Camera3RequestDescriptor { > >>> + enum class ProcessStatus { > >>> + None, > >>> + Processing, > >>> + Success, > >>> + Error > >>> + }; > >>> + > >>> Camera3RequestDescriptor() = default; > >>> ~Camera3RequestDescriptor() = default; > >>> Camera3RequestDescriptor(libcamera::Camera *camera, > >>> @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor { > >>> CameraMetadata settings_; > >>> std::unique_ptr<CaptureRequest> request_; > >>> std::unique_ptr<CameraMetadata> resultMetadata_; > >>> + > >>> + ProcessStatus status_; > >>> }; > >>> > >>> class CameraDevice : protected libcamera::Loggable > >>> @@ -79,6 +88,8 @@ public: > >>> int configureStreams(camera3_stream_configuration_t *stream_list); > >>> int processCaptureRequest(camera3_capture_request_t *request); > >>> void requestComplete(libcamera::Request *request); > >>> + void streamProcessingComplete(CameraStream *cameraStream, > >>> + Camera3RequestDescriptor *request); > >>> > >>> protected: > >>> std::string logPrefix() const override; > >>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > >>> index d494f5d5..70471959 100644 > >>> --- a/src/android/camera_stream.cpp > >>> +++ b/src/android/camera_stream.cpp > >>> @@ -81,6 +81,9 @@ int CameraStream::configure() > >>> int ret = postProcessor_->configure(configuration(), output); > >>> if (ret) > >>> return ret; > >>> + > >>> + postProcessor_->processComplete.connect( > >>> + this, &CameraStream::handleProcessComplete); > >>> } > >>> > >>> if (allocator_) { > >>> @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source, > >>> buffer_handle_t camera3Dest, > >>> Camera3RequestDescriptor *request) > >>> { > >>> - if (!postProcessor_) > >>> + if (!postProcessor_) { > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > >>> + handleProcessComplete(request); > >>> return 0; > >>> + } > >> > >> That was like this before, but I feel like this is a development > >> mistaken and should be better caught with Fatal > >> > >> same for the below !encoder and the YUV post-processor > > > > Or > > > > ASSERT(postProcessor_); > > > > ? > > > >>> /* > >>> * \todo Buffer mapping and processing should be moved to a > >>> @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source, > >>> PROT_READ | PROT_WRITE); > >>> if (!dest.isValid()) { > >>> LOG(HAL, Error) << "Failed to map android blob buffer"; > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > >>> + handleProcessComplete(request); > >>> return -EINVAL; > >>> } > >>> > >>> return postProcessor_->process(source, &dest, request); > >>> } > >>> > >>> +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request) > >>> +{ > >>> + cameraDevice_->streamProcessingComplete(this, request); > >>> +} > >> > >> Can't CameraStream connect CameraDevice::streamProcessingComplete with > >> the post-processor's processComplete signal ? > > > > We can do so with a lambda function, now supported by the signal class: > > > > postProcessor_->processComplete.connect( > > [&](Camera3RequestDescriptor *request) { > > cameraDevice_->streamProcessingComplete(this, request); > > } > > ); > > > >>> + > >>> FrameBuffer *CameraStream::getBuffer() > >>> { > >>> if (!allocator_) > >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > >>> index 68789700..f8ad6245 100644 > >>> --- a/src/android/camera_stream.h > >>> +++ b/src/android/camera_stream.h > >>> @@ -127,6 +127,8 @@ public: > >>> void putBuffer(libcamera::FrameBuffer *buffer); > >>> > >>> private: > >>> + void handleProcessComplete(Camera3RequestDescriptor *request); > >>> + > >>> CameraDevice *const cameraDevice_; > >>> const libcamera::CameraConfiguration *config_; > >>> const Type type_; > >>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > >>> index 31f68330..87252acd 100644 > >>> --- a/src/android/jpeg/post_processor_jpeg.cpp > >>> +++ b/src/android/jpeg/post_processor_jpeg.cpp > >>> @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > >>> CameraBuffer *destination, > >>> Camera3RequestDescriptor *request) > >>> { > >>> - if (!encoder_) > >>> + if (!encoder_) { > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > >>> + processComplete.emit(request); > >>> return 0; > >>> + } > >>> > >>> ASSERT(destination->numPlanes() == 1); > >>> > >>> @@ -197,6 +200,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > >>> exif.data(), quality); > >>> if (jpeg_size < 0) { > >>> LOG(JPEG, Error) << "Failed to encode stream image"; > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > >>> + processComplete.emit(request); > >>> return jpeg_size; > >>> } > >>> > >>> @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, > >>> /* Update the JPEG result Metadata. */ > >>> resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); > >>> > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > >>> + processComplete.emit(request); > >>> + > >>> return 0; > >>> } > >>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h > >>> index bdd6afe7..f639b237 100644 > >>> --- a/src/android/post_processor.h > >>> +++ b/src/android/post_processor.h > >>> @@ -7,6 +7,8 @@ > >>> #ifndef __ANDROID_POST_PROCESSOR_H__ > >>> #define __ANDROID_POST_PROCESSOR_H__ > >>> > >>> +#include <libcamera/base/signal.h> > >>> + > >>> #include <libcamera/framebuffer.h> > >>> #include <libcamera/stream.h> > >>> > >>> @@ -26,6 +28,8 @@ public: > >>> virtual int process(const libcamera::FrameBuffer *source, > >>> CameraBuffer *destination, > >>> Camera3RequestDescriptor *request) = 0; > >>> + > >>> + libcamera::Signal<Camera3RequestDescriptor *> processComplete; > >>> }; > >>> > >>> #endif /* __ANDROID_POST_PROCESSOR_H__ */ > >>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > >>> index 5e18caee..b144649a 100644 > >>> --- a/src/android/yuv/post_processor_yuv.cpp > >>> +++ b/src/android/yuv/post_processor_yuv.cpp > >>> @@ -18,6 +18,8 @@ > >>> #include "libcamera/internal/formats.h" > >>> #include "libcamera/internal/mapped_framebuffer.h" > >>> > >>> +#include "../camera_device.h" > >>> + > >>> using namespace libcamera; > >>> > >>> LOG_DEFINE_CATEGORY(YUV) > >>> @@ -51,14 +53,19 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, > >>> > >>> int PostProcessorYuv::process(const FrameBuffer *source, > >>> CameraBuffer *destination, > >>> - [[maybe_unused]] Camera3RequestDescriptor *request) > >>> + Camera3RequestDescriptor *request) > >>> { > >>> - if (!isValidBuffers(source, *destination)) > >>> + if (!isValidBuffers(source, *destination)) { > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > >>> + processComplete.emit(request); > >> > >> If my first suggestion will be considered, the post processor will > >> send the status in the signal, and you can reduce the number of status > >> to Success or Error maybe ? > >> > >>> return -EINVAL; > >>> + } > >>> > >>> const MappedFrameBuffer sourceMapped(source, MappedFrameBuffer::MapFlag::Read); > >>> if (!sourceMapped.isValid()) { > >>> LOG(YUV, Error) << "Failed to mmap camera frame buffer"; > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > >>> + processComplete.emit(request); > >>> return -EINVAL; > >>> } > >>> > >>> @@ -76,9 +83,14 @@ int PostProcessorYuv::process(const FrameBuffer *source, > >>> libyuv::FilterMode::kFilterBilinear); > >>> if (ret) { > >>> LOG(YUV, Error) << "Failed NV12 scaling: " << ret; > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; > >>> + processComplete.emit(request); > >>> return -EINVAL; > >>> } > >>> > >>> + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; > >>> + processComplete.emit(request); > >>> + > >>> return 0; > >>> } > >>>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index fa462368..e2de4012 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -246,6 +246,12 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( */ request_ = std::make_unique<CaptureRequest>(camera, reinterpret_cast<uint64_t>(this)); + + /* + * Denotes the post-processing status required by any stream. It is set + * to ProcessStatus::Success if processing is successful. + */ + status_ = ProcessStatus::None; } /* @@ -1150,6 +1156,8 @@ void CameraDevice::requestComplete(Request *request) continue; } + descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing; + int ret = cameraStream->process(src, *buffer.buffer, descriptor); /* @@ -1172,6 +1180,16 @@ void CameraDevice::requestComplete(Request *request) descriptors_.pop_front(); } + +void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream, + [[maybe_unused]] Camera3RequestDescriptor *request) +{ + /* + * \todo Stream processing is completed hence, check for errors and + * if any, mark the corresponding buffer with CAMERA3_BUFFER_STATUS_ERROR. + */ +} + std::string CameraDevice::logPrefix() const { return "'" + camera_->id() + "'"; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index b2871e52..60c134dc 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -36,6 +36,13 @@ struct CameraConfigData; struct Camera3RequestDescriptor { + enum class ProcessStatus { + None, + Processing, + Success, + Error + }; + Camera3RequestDescriptor() = default; ~Camera3RequestDescriptor() = default; Camera3RequestDescriptor(libcamera::Camera *camera, @@ -48,6 +55,8 @@ struct Camera3RequestDescriptor { CameraMetadata settings_; std::unique_ptr<CaptureRequest> request_; std::unique_ptr<CameraMetadata> resultMetadata_; + + ProcessStatus status_; }; class CameraDevice : protected libcamera::Loggable @@ -79,6 +88,8 @@ public: int configureStreams(camera3_stream_configuration_t *stream_list); int processCaptureRequest(camera3_capture_request_t *request); void requestComplete(libcamera::Request *request); + void streamProcessingComplete(CameraStream *cameraStream, + Camera3RequestDescriptor *request); protected: std::string logPrefix() const override; diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index d494f5d5..70471959 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -81,6 +81,9 @@ int CameraStream::configure() int ret = postProcessor_->configure(configuration(), output); if (ret) return ret; + + postProcessor_->processComplete.connect( + this, &CameraStream::handleProcessComplete); } if (allocator_) { @@ -102,8 +105,11 @@ int CameraStream::process(const FrameBuffer *source, buffer_handle_t camera3Dest, Camera3RequestDescriptor *request) { - if (!postProcessor_) + if (!postProcessor_) { + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; + handleProcessComplete(request); return 0; + } /* * \todo Buffer mapping and processing should be moved to a @@ -114,12 +120,19 @@ int CameraStream::process(const FrameBuffer *source, PROT_READ | PROT_WRITE); if (!dest.isValid()) { LOG(HAL, Error) << "Failed to map android blob buffer"; + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; + handleProcessComplete(request); return -EINVAL; } return postProcessor_->process(source, &dest, request); } +void CameraStream::handleProcessComplete(Camera3RequestDescriptor *request) +{ + cameraDevice_->streamProcessingComplete(this, request); +} + FrameBuffer *CameraStream::getBuffer() { if (!allocator_) diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h index 68789700..f8ad6245 100644 --- a/src/android/camera_stream.h +++ b/src/android/camera_stream.h @@ -127,6 +127,8 @@ public: void putBuffer(libcamera::FrameBuffer *buffer); private: + void handleProcessComplete(Camera3RequestDescriptor *request); + CameraDevice *const cameraDevice_; const libcamera::CameraConfiguration *config_; const Type type_; diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 31f68330..87252acd 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -101,8 +101,11 @@ int PostProcessorJpeg::process(const FrameBuffer *source, CameraBuffer *destination, Camera3RequestDescriptor *request) { - if (!encoder_) + if (!encoder_) { + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; + processComplete.emit(request); return 0; + } ASSERT(destination->numPlanes() == 1); @@ -197,6 +200,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, exif.data(), quality); if (jpeg_size < 0) { LOG(JPEG, Error) << "Failed to encode stream image"; + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; + processComplete.emit(request); return jpeg_size; } @@ -211,5 +216,8 @@ int PostProcessorJpeg::process(const FrameBuffer *source, /* Update the JPEG result Metadata. */ resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size); + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; + processComplete.emit(request); + return 0; } diff --git a/src/android/post_processor.h b/src/android/post_processor.h index bdd6afe7..f639b237 100644 --- a/src/android/post_processor.h +++ b/src/android/post_processor.h @@ -7,6 +7,8 @@ #ifndef __ANDROID_POST_PROCESSOR_H__ #define __ANDROID_POST_PROCESSOR_H__ +#include <libcamera/base/signal.h> + #include <libcamera/framebuffer.h> #include <libcamera/stream.h> @@ -26,6 +28,8 @@ public: virtual int process(const libcamera::FrameBuffer *source, CameraBuffer *destination, Camera3RequestDescriptor *request) = 0; + + libcamera::Signal<Camera3RequestDescriptor *> processComplete; }; #endif /* __ANDROID_POST_PROCESSOR_H__ */ diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp index 5e18caee..b144649a 100644 --- a/src/android/yuv/post_processor_yuv.cpp +++ b/src/android/yuv/post_processor_yuv.cpp @@ -18,6 +18,8 @@ #include "libcamera/internal/formats.h" #include "libcamera/internal/mapped_framebuffer.h" +#include "../camera_device.h" + using namespace libcamera; LOG_DEFINE_CATEGORY(YUV) @@ -51,14 +53,19 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg, int PostProcessorYuv::process(const FrameBuffer *source, CameraBuffer *destination, - [[maybe_unused]] Camera3RequestDescriptor *request) + Camera3RequestDescriptor *request) { - if (!isValidBuffers(source, *destination)) + if (!isValidBuffers(source, *destination)) { + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; + processComplete.emit(request); return -EINVAL; + } const MappedFrameBuffer sourceMapped(source, MappedFrameBuffer::MapFlag::Read); if (!sourceMapped.isValid()) { LOG(YUV, Error) << "Failed to mmap camera frame buffer"; + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; + processComplete.emit(request); return -EINVAL; } @@ -76,9 +83,14 @@ int PostProcessorYuv::process(const FrameBuffer *source, libyuv::FilterMode::kFilterBilinear); if (ret) { LOG(YUV, Error) << "Failed NV12 scaling: " << ret; + request->status_ = Camera3RequestDescriptor::ProcessStatus::Error; + processComplete.emit(request); return -EINVAL; } + request->status_ = Camera3RequestDescriptor::ProcessStatus::Success; + processComplete.emit(request); + return 0; }
We should be able to know if post-processing has been completed successfully or encountered some errors. This commit introduces a Signal<> that will notify that the post-processing has been completed. If the post processing was successful, status on the request descriptor will be set to Camera3RequestDescriptor::ProcessStatus::Success. The signal will be required when the post-processor is meant to run asynchronously (in subsequent commits) and capture results need to be sent back to the framework from the signal's slot instead. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/camera_device.cpp | 18 ++++++++++++++++++ src/android/camera_device.h | 11 +++++++++++ src/android/camera_stream.cpp | 15 ++++++++++++++- src/android/camera_stream.h | 2 ++ src/android/jpeg/post_processor_jpeg.cpp | 10 +++++++++- src/android/post_processor.h | 4 ++++ src/android/yuv/post_processor_yuv.cpp | 16 ++++++++++++++-- 7 files changed, 72 insertions(+), 4 deletions(-)