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

[Umang Jain]

Hi Laurent,

On 9/13/21 5:49 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> 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.


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


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