[libcamera-devel] [PATCH v3 07/10] android: post_processor: Notify post processing completion status
Umang Jain
umang.jain at ideasonboard.com
Wed Sep 22 10:43:56 CEST 2021
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 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
- 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;
>>> }
>>>
More information about the libcamera-devel
mailing list