[libcamera-devel] [PATCH v4 4/7] android: camera_stream: Drop return value for process()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 12 05:39:55 CEST 2021
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.
> }
> 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 ?
>
> /*
> * \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.
>
> - 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,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list