[libcamera-devel] [PATCH v2 5/9] android: camera_stream: Query post-processing status

[Laurent Pinchart]

Hi Umang,

On Mon, Sep 13, 2021 at 11:55:35AM +0530, Umang Jain wrote:
> On 9/13/21 5:49 AM, Laurent Pinchart wrote:
> > On Fri, Sep 10, 2021 at 12:36:34PM +0530, Umang Jain wrote:
> >> CameraStream takes care of any post-processing required via the
> >> CameraStream::process(). Since the post-processor will be moved
> >> to a separate thread (in subsequent commits), the caller of
> >> PostProcessor::process() will notify the associated CameraStream
> >> instance about the post-processing completion via processComplete
> >> signal. The CameraStream class should handle this event to query
> >> the status and take next steps of reporting it back to upper layers
> >> (i.e. CameraDevice).
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >>   src/android/camera_stream.cpp | 13 +++++++++++++
> >>   src/android/camera_stream.h   | 13 ++++++++++++-
> >>   2 files changed, 25 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >> index d1c54797..996779c4 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::postProcessingComplete);
> >>   	}
> >>   
> >>   	if (allocator_) {
> >> @@ -123,6 +126,16 @@ int CameraStream::process(const FrameBuffer *source,
> >>   				       resultMetadata, context);
> >>   }
> >>   
> >> +void CameraStream::postProcessingComplete(PostProcessor::Status status,
> >> +					  const Camera3RequestDescriptor *context)
> >> +{
> >> +	ProcessStatus processStatus;
> >> +	if (status == PostProcessor::Status::Succeeded)
> >> +		processStatus = ProcessStatus::Succeeded;
> >> +	else
> >> +		processStatus = ProcessStatus::Failed;
> >> +}
> >
> > I'd squash this patch with 6/9, it doesn't do much on its own, and fails
> > to compile due to the unused processStatus variable.
> >
> >> +
> >>   FrameBuffer *CameraStream::getBuffer()
> >>   {
> >>   	if (!allocator_)
> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> >> index d589f699..be076995 100644
> >> --- a/src/android/camera_stream.h
> >> +++ b/src/android/camera_stream.h
> >> @@ -13,15 +13,18 @@
> >>   
> >>   #include <hardware/camera3.h>
> >>   
> >> +#include <libcamera/base/signal.h>
> >
> > I don't think you need the signal header here.
> >
> >> +
> >>   #include <libcamera/camera.h>
> >>   #include <libcamera/framebuffer.h>
> >>   #include <libcamera/framebuffer_allocator.h>
> >>   #include <libcamera/geometry.h>
> >>   #include <libcamera/pixel_format.h>
> >>   
> >> +#include "post_processor.h"
> >> +
> >>   class CameraDevice;
> >>   class CameraMetadata;
> >> -class PostProcessor;
> >>   
> >>   struct Camera3RequestDescriptor;
> >>   
> >> @@ -128,7 +131,15 @@ public:
> >>   	libcamera::FrameBuffer *getBuffer();
> >>   	void putBuffer(libcamera::FrameBuffer *buffer);
> >>   
> >> +	enum ProcessStatus {
> >> +		Succeeded,
> >> +		Failed,
> >> +	};
> >
> > Do we need this enum, could we use PostProcessor::Status everywhere ?
> 
> Yes, that would be my preference as well. But I think is if we do, we 
> will have PostProcessor:: in CameraDevice, which currently doesn't 
> interact with PostProcessor layer atleast directly. And I would like to 
> keep it that way.

I was expecting that objection :-)

> Maybe keep the status enum in CameraStream class and use it both in 
> PostProcessor and CameraDevice. That way we achieve Status:: stuff 
> everywhere probably.

How about adding the enum to Camera3RequestDescriptor ? It's a request
completion status after all, so it would make sense to centralize it
there.

> >> +
> >>   private:
> >> +	void postProcessingComplete(PostProcessor::Status status,
> >> +				    const Camera3RequestDescriptor *context);
> >> +
> >>   	CameraDevice *const cameraDevice_;
> >>   	const libcamera::CameraConfiguration *config_;
> >>   	const Type type_;

-- 
Regards,

Laurent Pinchart