[libcamera-devel] [PATCH v4 4/7] android: camera_stream: Drop return value for process()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 13 12:14:00 CEST 2021
Hi Umang,
On Wed, Oct 13, 2021 at 03:33:00PM +0530, Umang Jain wrote:
> On 10/12/21 9:09 AM, Laurent Pinchart wrote:
> > 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.
You can have a local variable that indicates if at least one stream
required post-processing, and return after this loop 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)
If it can't happen unless there's a bug in the HAL, ASSERT() will likely
be easier, yes. Otherwise you need to notify an error to the caller or
the request will be lost silently.
> >>
> >> /*
> >> * \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)
To avoid calling that in every single location here, I think
CameraStream::process() should return an error status. The caller can
then handle it. It's like queuing a request, errors can be returned
synchronously, but if the call succeeds, then asynchronous errors are
notified through a different mechanism.
> 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,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list