[libcamera-devel] [PATCH v4 4/7] android: camera_stream: Drop return value for process()
Umang Jain
umang.jain at ideasonboard.com
Wed Oct 13 12:03:00 CEST 2021
Hi Laurent,
On 10/12/21 9:09 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Oct 11, 2021 at 01:05:02PM +0530, Umang Jain wrote:
>> CameraStream::process() is invoked by CameraDevice::requestComplete()
>> in case any post-processing is required for the camera stream. The
>> failure or success is checked via the value returned by
>> CameraStream::process().
>>
>> Now that the post-processor notifies about the post-processing
>> completion operation, we can drop the return value of
>> CameraStream::process(). The status of post-processing is passed
>> to CameraDevice::streamProcessingComplete() by the
>> PostProcessor::processComplete slot.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> src/android/camera_device.cpp | 2 +-
>> src/android/camera_stream.cpp | 14 +++++++-------
>> src/android/camera_stream.h | 6 +++---
>> src/android/jpeg/post_processor_jpeg.cpp | 12 +++++-------
>> src/android/jpeg/post_processor_jpeg.h | 6 +++---
>> src/android/post_processor.h | 6 +++---
>> src/android/yuv/post_processor_yuv.cpp | 14 ++++++--------
>> src/android/yuv/post_processor_yuv.h | 6 +++---
>> 8 files changed, 31 insertions(+), 35 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 9f26c36d..eba370ea 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1228,7 +1228,7 @@ void CameraDevice::requestComplete(Request *request)
>> if (cameraStream->type() == CameraStream::Type::Internal)
>> descriptor->internalBuffer_ = src;
>>
>> - int ret = cameraStream->process(*src, buffer, descriptor);
>> + cameraStream->process(*src, buffer, descriptor);
>>
>> return;
> I forgot to comment in the previous patch that you should only return
> early if no stream requires post-processing, not after the first one.
We can only know that by iterating over the descriptor->buffers_ right?
We return early here since, we don't want to trigger
sendCaptureResults() below, outside the loop.
Since we are now discussing multiple post-processing requests even for
single requestComplete(), this loop becomes crucial as it will queue up
all the post-processing requests to relevant cameraStreams
I'll think how should we return in that case.
>
>> }
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index d91e7dee..cec07269 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -147,9 +147,9 @@ int CameraStream::waitFence(int fence)
>> return -errno;
>> }
>>
>> -int CameraStream::process(const FrameBuffer &source,
>> - camera3_stream_buffer_t &camera3Dest,
>> - Camera3RequestDescriptor *request)
>> +void CameraStream::process(const FrameBuffer &source,
>> + camera3_stream_buffer_t &camera3Dest,
>> + Camera3RequestDescriptor *request)
>> {
>> /* Handle waiting on fences on the destination buffer. */
>> int fence = camera3Dest.acquire_fence;
>> @@ -160,12 +160,12 @@ int CameraStream::process(const FrameBuffer &source,
>> if (ret < 0) {
>> LOG(HAL, Error) << "Failed waiting for fence: "
>> << fence << ": " << strerror(-ret);
>> - return ret;
>> + return;
>> }
>> }
>>
>> if (!postProcessor_)
>> - return 0;
>> + return;
> Can this happen btw ?
If it happens, it'll be the caller's error for triggering it.
A CameraStream Type::Direct and you call a ->process() on it. Doesn't
make sense right? Should be up the intensity and put an assert instead?
ASSERT (type_ != Type::Direct)
>
>>
>> /*
>> * \todo Buffer mapping and processing should be moved to a
>> @@ -176,10 +176,10 @@ int CameraStream::process(const FrameBuffer &source,
>> PROT_READ | PROT_WRITE);
>> if (!dest.isValid()) {
>> LOG(HAL, Error) << "Failed to create destination buffer";
>> - return -EINVAL;
>> + return;
>> }
> We have a set of failure conditions here that don't result in any
> notification to the CameraDevice, the request will be lost. You could
> call CameraDevice::streamProcessingComplete() in those locations, but it
> may be easier to keep returning an int from this function (not the ones
> below) and handle errors in the caller.
Every error handle patch should call
cameraDevice_->streamProcessingComplete(this, request,
Camera3RequestDescriptor::Status::Error)
This is my mistake not to closely follow the error handling paths in
this patch
>
>>
>> - return postProcessor_->process(source, &dest, request);
>> + postProcessor_->process(source, &dest, request);
>> }
>>
>> FrameBuffer *CameraStream::getBuffer()
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index 04cfd111..a0c5f166 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -120,9 +120,9 @@ public:
>> libcamera::Stream *stream() const;
>>
>> int configure();
>> - int process(const libcamera::FrameBuffer &source,
>> - camera3_stream_buffer_t &camera3Buffer,
>> - Camera3RequestDescriptor *request);
>> + void process(const libcamera::FrameBuffer &source,
>> + camera3_stream_buffer_t &camera3Buffer,
>> + Camera3RequestDescriptor *request);
>> libcamera::FrameBuffer *getBuffer();
>> void putBuffer(libcamera::FrameBuffer *buffer);
>>
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index 81d1efe6..eb87931b 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -97,12 +97,12 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>> }
>> }
>>
>> -int PostProcessorJpeg::process(const FrameBuffer &source,
>> - CameraBuffer *destination,
>> - Camera3RequestDescriptor *request)
>> +void PostProcessorJpeg::process(const FrameBuffer &source,
>> + CameraBuffer *destination,
>> + Camera3RequestDescriptor *request)
>> {
>> if (!encoder_)
>> - return 0;
>> + return;
> Same here, if this can happen, we'll lose the request, if it can't
> happen, it should be dropped, or replaced with an ASSERT() if you're not
> sure.
>
>>
>> ASSERT(destination->numPlanes() == 1);
>>
>> @@ -198,7 +198,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>> if (jpeg_size < 0) {
>> LOG(JPEG, Error) << "Failed to encode stream image";
>> processComplete.emit(request, PostProcessor::Status::Error);
>> - return jpeg_size;
>> + return;
>> }
>>
>> /* Fill in the JPEG blob header. */
>> @@ -212,6 +212,4 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>> /* Update the JPEG result Metadata. */
>> resultMetadata->addEntry(ANDROID_JPEG_SIZE, jpeg_size);
>> processComplete.emit(request, PostProcessor::Status::Success);
>> -
>> - return 0;
>> }
>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> index 0184d77e..5dab14e1 100644
>> --- a/src/android/jpeg/post_processor_jpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -22,9 +22,9 @@ public:
>>
>> int configure(const libcamera::StreamConfiguration &incfg,
>> const libcamera::StreamConfiguration &outcfg) override;
>> - int process(const libcamera::FrameBuffer &source,
>> - CameraBuffer *destination,
>> - Camera3RequestDescriptor *request) override;
>> + void process(const libcamera::FrameBuffer &source,
>> + CameraBuffer *destination,
>> + Camera3RequestDescriptor *request) override;
>>
>> private:
>> void generateThumbnail(const libcamera::FrameBuffer &source,
>> diff --git a/src/android/post_processor.h b/src/android/post_processor.h
>> index 6e67bcba..88a5f985 100644
>> --- a/src/android/post_processor.h
>> +++ b/src/android/post_processor.h
>> @@ -30,9 +30,9 @@ public:
>>
>> virtual int configure(const libcamera::StreamConfiguration &inCfg,
>> const libcamera::StreamConfiguration &outCfg) = 0;
>> - virtual int process(const libcamera::FrameBuffer &source,
>> - CameraBuffer *destination,
>> - Camera3RequestDescriptor *request) = 0;
>> + virtual void process(const libcamera::FrameBuffer &source,
>> + CameraBuffer *destination,
>> + Camera3RequestDescriptor *request) = 0;
>>
>> libcamera::Signal<Camera3RequestDescriptor *, Status> processComplete;
>> };
>> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp
>> index b34b389d..860a1a7f 100644
>> --- a/src/android/yuv/post_processor_yuv.cpp
>> +++ b/src/android/yuv/post_processor_yuv.cpp
>> @@ -51,20 +51,20 @@ int PostProcessorYuv::configure(const StreamConfiguration &inCfg,
>> return 0;
>> }
>>
>> -int PostProcessorYuv::process(const FrameBuffer &source,
>> - CameraBuffer *destination,
>> - Camera3RequestDescriptor *request)
>> +void PostProcessorYuv::process(const FrameBuffer &source,
>> + CameraBuffer *destination,
>> + Camera3RequestDescriptor *request)
>> {
>> if (!isValidBuffers(source, *destination)) {
>> processComplete.emit(request, PostProcessor::Status::Error);
>> - return -EINVAL;
>> + return;
>> }
>>
>> const MappedFrameBuffer sourceMapped(&source, MappedFrameBuffer::MapFlag::Read);
>> if (!sourceMapped.isValid()) {
>> LOG(YUV, Error) << "Failed to mmap camera frame buffer";
>> processComplete.emit(request, PostProcessor::Status::Error);
>> - return -EINVAL;
>> + return;
>> }
>>
>> int ret = libyuv::NV12Scale(sourceMapped.planes()[0].data(),
>> @@ -82,12 +82,10 @@ int PostProcessorYuv::process(const FrameBuffer &source,
>> if (ret) {
>> LOG(YUV, Error) << "Failed NV12 scaling: " << ret;
>> processComplete.emit(request, PostProcessor::Status::Error);
>> - return -EINVAL;
>> + return;
>> }
>>
>> processComplete.emit(request, PostProcessor::Status::Success);
>> -
>> - return 0;
>> }
>>
>> bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source,
>> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h
>> index a4e0ff5d..1278843b 100644
>> --- a/src/android/yuv/post_processor_yuv.h
>> +++ b/src/android/yuv/post_processor_yuv.h
>> @@ -18,9 +18,9 @@ public:
>>
>> int configure(const libcamera::StreamConfiguration &incfg,
>> const libcamera::StreamConfiguration &outcfg) override;
>> - int process(const libcamera::FrameBuffer &source,
>> - CameraBuffer *destination,
>> - Camera3RequestDescriptor *request) override;
>> + void process(const libcamera::FrameBuffer &source,
>> + CameraBuffer *destination,
>> + Camera3RequestDescriptor *request) override;
>>
>> private:
>> bool isValidBuffers(const libcamera::FrameBuffer &source,
More information about the libcamera-devel
mailing list