[libcamera-devel] [PATCH v3 07/10] android: post_processor: Notify post processing completion status
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 22 10:50:04 CEST 2021
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 at 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;
> >>> }
> >>>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list